|
|
Created:
8 years, 5 months ago by ondrejnovak Modified:
8 years, 5 months ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionDo not swallow inner startup exceptions
Patch Set 1 #Patch Set 2 : toString version #MessagesTotal messages: 9
+Brett
Sign in to reply to this message.
Why did you find this necessary? This code already uses a log-and-throw anti-pattern to force the exception to be logged. The re-throw at this point is used to force the application to exit. Adding the original exception as the cause will just get it logged to the console again on the way out.
Sign in to reply to this message.
On 2015/10/08 19:32:30, Brett wrote: > Why did you find this necessary? This code already uses a log-and-throw > anti-pattern to force the exception to be logged. The re-throw at this point is > used to force the application to exit. Adding the original exception as the > cause will just get it logged to the console again on the way out. I have uploaded version with e.toString() as well, but I personally prefer the original one.
Sign in to reply to this message.
LGTM with e.toString(). That turned out to be less than useful in this particular instance because of who threw the exception. I will submit a separate CL that validates the server path configuration parameter and produces a more meaningful message.
Sign in to reply to this message.
Also, please edit the commit message a bit to be more specific about the change. The root cause exception is not swallowed, but logged. It simply wasn't passed to the StartupException that terminated the thread (which is what the JVM printed on the console).
Sign in to reply to this message.
Change the commit message to below. However I do not have access to the library repository. Can you grant it to me? Thanks Print inner startup exception cause to stderr Startup exceptions should have the cause printed to the stderr. Currently the full stack trace is printed to the log file, and only startup exception to the stderr - looking like this Exception in thread "main" com.google.enterprise.adaptor.StartupException: Failed to start application. at com.google.enterprise.adaptor.Application.main(Application.java:555) at com.google.enterprise.adaptor.AbstractAdaptor.main(AbstractAdaptor.java:61) at com.google.enterprise.adaptor.database.DatabaseAdaptor.main(DatabaseAdaptor.java:271) Especially when there are some other warnings printed out to stdout/stderr, this can be confusing, so including also the inner exception in the message On Sat, Oct 10, 2015 at 3:13 AM, <jlacey@google.com> wrote: > Also, please edit the commit message a bit to be more specific about the > change. The root cause exception is not swallowed, but logged. It simply > wasn't passed to the StartupException that terminated the thread (which > is what the JVM printed on the console). > > https://codereview.appspot.com/261710043/ >
Sign in to reply to this message.
github user ondrejnovak now has write access to googlegsa/library Thank you, PJ On 2015/10/12 10:51:22, ondrejnovak wrote: > Change the commit message to below. However I do not have access to the > library repository. Can you grant it to me? Thanks > > Print inner startup exception cause to stderr > > Startup exceptions should have the cause printed to the stderr. > Currently the full stack trace is printed to the log file, and only startup > exception to the stderr - looking like this > > Exception in thread "main" > com.google.enterprise.adaptor.StartupException: Failed to start application. > at > com.google.enterprise.adaptor.Application.main(Application.java:555) > at > com.google.enterprise.adaptor.AbstractAdaptor.main(AbstractAdaptor.java:61) > at > com.google.enterprise.adaptor.database.DatabaseAdaptor.main(DatabaseAdaptor.java:271) > > Especially when there are some other warnings printed out to > stdout/stderr, this can be confusing, so including also the inner exception > in the message > > > > On Sat, Oct 10, 2015 at 3:13 AM, <mailto:jlacey@google.com> wrote: > > > Also, please edit the commit message a bit to be more specific about the > > change. The root cause exception is not swallowed, but logged. It simply > > wasn't passed to the StartupException that terminated the thread (which > > is what the JVM printed on the console). > > > > https://codereview.appspot.com/261710043/ > >
Sign in to reply to this message.
Thanks, pushed as https://github.com/googlegsa/library/commit/0ec3148b1c9cf1ed5ae52573004eec988... On Mon, Oct 12, 2015 at 7:56 PM, <pjo@google.com> wrote: > > github user ondrejnovak now has write > access to googlegsa/library > > Thank you, > PJ > > > > On 2015/10/12 10:51:22, ondrejnovak wrote: > >> Change the commit message to below. However I do not have access to >> > the > >> library repository. Can you grant it to me? Thanks >> > > Print inner startup exception cause to stderr >> > > Startup exceptions should have the cause printed to the stderr. >> Currently the full stack trace is printed to the log file, and only >> > startup > >> exception to the stderr - looking like this >> > > Exception in thread "main" >> com.google.enterprise.adaptor.StartupException: Failed to start >> > application. > >> at >> com.google.enterprise.adaptor.Application.main(Application.java:555) >> at >> > > com.google.enterprise.adaptor.AbstractAdaptor.main(AbstractAdaptor.java:61) > >> at >> > > > com.google.enterprise.adaptor.database.DatabaseAdaptor.main(DatabaseAdaptor.java:271) > > Especially when there are some other warnings printed out to >> stdout/stderr, this can be confusing, so including also the inner >> > exception > >> in the message >> > > > > On Sat, Oct 10, 2015 at 3:13 AM, <mailto:jlacey@google.com> wrote: >> > > > Also, please edit the commit message a bit to be more specific about >> > the > >> > change. The root cause exception is not swallowed, but logged. It >> > simply > >> > wasn't passed to the StartupException that terminated the thread >> > (which > >> > is what the JVM printed on the console). >> > >> > https://codereview.appspot.com/261710043/ >> > >> > > > https://codereview.appspot.com/261710043/ >
Sign in to reply to this message.
|