Issue 88040049: FSA Change Exceptions generated in FsAdaptor.init() to new StartupExceptions. (Closed)
|
Created:
10 years ago by Brett Modified:
9 years, 11 months ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionThis forces an abort of the adaptor startup, rather than allowing it
to be called again with backoff logic.
Patch Set 1 #Patch Set 2 : Switch to using the new StartupExceptions. #
Total comments: 12
Patch Set 3 : PJ and Miguel's feedback #
Total comments: 2
Patch Set 4 : Change error message. #
MessagesTotal messages: 20
Just want to make sure that you took this out for a spin and that library doesn't catch ISE. If it does catch ISE, then we can do something like AssertionError, which it won't catch. I guess we want to make sure that the process really does stop. Thank you, PJ
Sign in to reply to this message.
I tried it with empty root path. Unfortunately, IllegalStateException also retries. Looking at Application.daemonStart(), it catches *all* Exception (except InterruptedException) and retries. I could be convinced that IllegalStateException, IllegalArgumentException, etc should be handled explicitly in Application.daemonStart(). Throwing an AssertionError does not retry, but it doesn’t exit either. — Brett M. Johnson On Apr 16, 2014, at 1:48 PM, pjo@google.com wrote: > Just want to make sure that you took this out > for a spin and that library doesn't catch ISE. > > If it does catch ISE, then we can do something > like AssertionError, which it won't catch. > > I guess we want to make sure that the process > really does stop. > > Thank you, > PJ > > https://codereview.appspot.com/88040049/
Sign in to reply to this message.
A few ideas are going through my mind. How about we add `class Abort extends Exception" to library and have init throw it? And library does not do retries if that is the exception that go thrown? This exception would be for things like "wrong platform" and "horrible config". Thank you, PJ - technology's compounding interest - On Wed, Apr 16, 2014 at 2:07 PM, Brett Johnson < brett.michael.johnson@gmail.com> wrote: > I tried it with empty root path. > Unfortunately, IllegalStateException also retries. > Looking at Application.daemonStart(), it catches > *all* Exception (except InterruptedException) and > retries. > > I could be convinced that IllegalStateException, > IllegalArgumentException, etc should be handled > explicitly in Application.daemonStart(). > > Throwing an AssertionError does not retry, > but it doesn't exit either. > > -- > Brett M. Johnson > > On Apr 16, 2014, at 1:48 PM, pjo@google.com wrote: > > > Just want to make sure that you took this out > > for a spin and that library doesn't catch ISE. > > > > If it does catch ISE, then we can do something > > like AssertionError, which it won't catch. > > > > I guess we want to make sure that the process > > really does stop. > > > > Thank you, > > PJ > > > > https://codereview.appspot.com/88040049/ > >
Sign in to reply to this message.
Added task to backlog for next iteration to add something along the lines of "InvalidSetupException" and have it be a checked excption throw by init(). This exception would be for things like wrong platform, or horrible config. This would not be for potentially temporary or fixable conditons like server down or insufficient ACLs. Thank you, PJ - technology's compounding interest - On Wed, Apr 16, 2014 at 2:18 PM, PJ <pjo@google.com> wrote: > A few ideas are going through my mind. > > How about we add `class Abort extends Exception" > to library and have init throw it? And library does > not do retries if that is the exception that go thrown? > > This exception would be for things like "wrong platform" > and "horrible config". > > Thank you, > PJ > > > > > - technology's compounding interest > - > > > On Wed, Apr 16, 2014 at 2:07 PM, Brett Johnson < > brett.michael.johnson@gmail.com> wrote: > >> I tried it with empty root path. >> Unfortunately, IllegalStateException also retries. >> Looking at Application.daemonStart(), it catches >> *all* Exception (except InterruptedException) and >> retries. >> >> I could be convinced that IllegalStateException, >> IllegalArgumentException, etc should be handled >> explicitly in Application.daemonStart(). >> >> Throwing an AssertionError does not retry, >> but it doesn't exit either. >> >> -- >> Brett M. Johnson >> >> On Apr 16, 2014, at 1:48 PM, pjo@google.com wrote: >> >> > Just want to make sure that you took this out >> > for a spin and that library doesn't catch ISE. >> > >> > If it does catch ISE, then we can do something >> > like AssertionError, which it won't catch. >> > >> > I guess we want to make sure that the process >> > really does stop. >> > >> > Thank you, >> > PJ >> > >> > https://codereview.appspot.com/88040049/ >> >> >
Sign in to reply to this message.
Shall I leave this CL open, with an impending patch that will throw InvalidSetupException? — Brett M. Johnson On Apr 16, 2014, at 2:57 PM, PJ <pjo@google.com> wrote: > Added task to backlog for next iteration to add something > along the lines of "InvalidSetupException" and have it be > a checked excption throw by init(). > > This exception would be for things like wrong platform, > or horrible config. This would not be for potentially temporary > or fixable conditons like server down or insufficient ACLs. > > Thank you, > PJ > > > > > - technology's compounding interest > - > > > On Wed, Apr 16, 2014 at 2:18 PM, PJ <pjo@google.com> wrote: > A few ideas are going through my mind. > > How about we add `class Abort extends Exception" > to library and have init throw it? And library does > not do retries if that is the exception that go thrown? > > This exception would be for things like "wrong platform" > and "horrible config". > > Thank you, > PJ > > > > > - technology's compounding interest > - > > > On Wed, Apr 16, 2014 at 2:07 PM, Brett Johnson <brett.michael.johnson@gmail.com> wrote: > I tried it with empty root path. > Unfortunately, IllegalStateException also retries. > Looking at Application.daemonStart(), it catches > *all* Exception (except InterruptedException) and > retries. > > I could be convinced that IllegalStateException, > IllegalArgumentException, etc should be handled > explicitly in Application.daemonStart(). > > Throwing an AssertionError does not retry, > but it doesn’t exit either. > > — > Brett M. Johnson > > On Apr 16, 2014, at 1:48 PM, pjo@google.com wrote: > > > Just want to make sure that you took this out > > for a spin and that library doesn't catch ISE. > > > > If it does catch ISE, then we can do something > > like AssertionError, which it won't catch. > > > > I guess we want to make sure that the process > > really does stop. > > > > Thank you, > > PJ > > > > https://codereview.appspot.com/88040049/ > > >
Sign in to reply to this message.
Whatever is more convenient for you. LGTM. Thank you, PJ - technology's compounding interest - On Wed, Apr 16, 2014 at 3:10 PM, Brett Johnson < brett.michael.johnson@gmail.com> wrote: > Shall I leave this CL open, with an impending patch that will > throw InvalidSetupException? > > -- > Brett M. Johnson > > On Apr 16, 2014, at 2:57 PM, PJ <pjo@google.com> wrote: > > Added task to backlog for next iteration to add something > along the lines of "InvalidSetupException" and have it be > a checked excption throw by init(). > > This exception would be for things like wrong platform, > or horrible config. This would not be for potentially temporary > or fixable conditons like server down or insufficient ACLs. > > Thank you, > PJ > > > > > - technology's compounding interest > - > > > On Wed, Apr 16, 2014 at 2:18 PM, PJ <pjo@google.com> wrote: > >> A few ideas are going through my mind. >> >> How about we add `class Abort extends Exception" >> to library and have init throw it? And library does >> not do retries if that is the exception that go thrown? >> >> This exception would be for things like "wrong platform" >> and "horrible config". >> >> Thank you, >> PJ >> >> >> >> >> - technology's compounding interest >> - >> >> >> On Wed, Apr 16, 2014 at 2:07 PM, Brett Johnson < >> brett.michael.johnson@gmail.com> wrote: >> >>> I tried it with empty root path. >>> Unfortunately, IllegalStateException also retries. >>> Looking at Application.daemonStart(), it catches >>> *all* Exception (except InterruptedException) and >>> retries. >>> >>> I could be convinced that IllegalStateException, >>> IllegalArgumentException, etc should be handled >>> explicitly in Application.daemonStart(). >>> >>> Throwing an AssertionError does not retry, >>> but it doesn't exit either. >>> >>> -- >>> Brett M. Johnson >>> >>> On Apr 16, 2014, at 1:48 PM, pjo@google.com wrote: >>> >>> > Just want to make sure that you took this out >>> > for a spin and that library doesn't catch ISE. >>> > >>> > If it does catch ISE, then we can do something >>> > like AssertionError, which it won't catch. >>> > >>> > I guess we want to make sure that the process >>> > really does stop. >>> > >>> > Thank you, >>> > PJ >>> > >>> > https://codereview.appspot.com/88040049/ >>> >>> >> > >
Sign in to reply to this message.
Switch to using the new StartupExceptions.
Sign in to reply to this message.
Thank you. A couple of comments. Miguel, please also review this CL. Thank you, PJ https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:247: throw new StartupException("Unable to list the contents of " + rootPath + I think maybe this one should stay as IO. https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:280: throw new StartupException("Unable to read ACLs for " + rootPath + Also leave as IO. https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:290: } which of the above conditions happens when the server is being restarted? Is it line #238? In which case that should also stay to be IOException.
Sign in to reply to this message.
I'm a little confused about the flow for Adaptor.init when there is an error. Right now Adaptor.init on failure gets re-called with backoff. This works well when the adaptor is running as a service and machines are rebooted because it could be possible that FsAdaptor.init fails but 10 seconds later passes because of network issues or rebooted or whatever. With use of InvalidConfigurationException this fails which kind of hurts the use of the FS adaptor as a service since someone will have to manually restart the service and FsAdaptor.init failure. https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:247: throw new StartupException("Unable to list the contents of " + rootPath + Why not InvalidConfigurationException? On 2014/05/08 02:12:15, pjo wrote: > I think maybe this one should stay as IO. https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:280: throw new StartupException("Unable to read ACLs for " + rootPath + These last 3, 2 use InvalidConfigurationException and 1 uses StartupException. I think they should all use the same exception since they're all permission/access checks. On 2014/05/08 02:12:15, pjo wrote: > Also leave as IO.
Sign in to reply to this message.
Hi Miguel, All the places which are potentailly fixable without config change shouldn't be using the InvalidConfigException. For any spot that you see that is recoverable please identify it in this CL so that we can put it back as a recoverable exception like IOException. Thank you - technology's compounding interest - On Wed, May 7, 2014 at 9:18 PM, <mifern@google.com> wrote: > I'm a little confused about the flow for Adaptor.init when there is an > error. Right now Adaptor.init on failure gets re-called with backoff. > This works well when the adaptor is running as a service and machines > are rebooted because it could be possible that FsAdaptor.init fails but > 10 seconds later passes because of network issues or rebooted or > whatever. With use of InvalidConfigurationException this fails which > kind of hurts the use of the FS adaptor as a service since someone will > have to manually restart the service and FsAdaptor.init failure. > > > > https://codereview.appspot.com/88040049/diff/20001/src/ > com/google/enterprise/adaptor/fs/FsAdaptor.java > File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): > > https://codereview.appspot.com/88040049/diff/20001/src/ > com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode247 > src/com/google/enterprise/adaptor/fs/FsAdaptor.java:247: throw new > StartupException("Unable to list the contents of " + rootPath + > Why not InvalidConfigurationException? > > > On 2014/05/08 02:12:15, pjo wrote: > >> I think maybe this one should stay as IO. >> > > https://codereview.appspot.com/88040049/diff/20001/src/ > com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode280 > src/com/google/enterprise/adaptor/fs/FsAdaptor.java:280: throw new > StartupException("Unable to read ACLs for " + rootPath + > These last 3, 2 use InvalidConfigurationException and 1 uses > StartupException. I think they should all use the same exception since > they're all permission/access checks. > > > On 2014/05/08 02:12:15, pjo wrote: > >> Also leave as IO. >> > > https://codereview.appspot.com/88040049/ >
Sign in to reply to this message.
https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:247: throw new StartupException("Unable to list the contents of " + rootPath + This is recoverable so should probably stay IO. https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:270: throw new InvalidConfigurationException("The path " + rootPath + " is " I'm not sure about this one but I would also lean towards leaving it as IO. https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:280: throw new StartupException("Unable to read ACLs for " + rootPath + This is also recoverable so should probably stay IO.
Sign in to reply to this message.
https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:247: throw new StartupException("Unable to list the contents of " + rootPath + On 2014/05/08 14:11:46, mifern wrote: > This is recoverable so should probably stay IO. Done. https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:270: throw new InvalidConfigurationException("The path " + rootPath + " is " On 2014/05/08 14:11:46, mifern wrote: > I'm not sure about this one but I would also lean towards leaving it as IO. I don't think so. Unless you change the configuration, no content will be indexed. https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:280: throw new StartupException("Unable to read ACLs for " + rootPath + On 2014/05/08 14:11:46, mifern wrote: > This is also recoverable so should probably stay IO. Done. https://codereview.appspot.com/88040049/diff/20001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:290: } On 2014/05/08 02:12:15, pjo wrote: > which of the above conditions happens when the server > is being restarted? Is it line #238? In which case > that should also stay to be IOException. I suspect that would indeed be the first error, so I converted it to an IOException.
Sign in to reply to this message.
PJ and Miguel's feedback
Sign in to reply to this message.
Thank you. https://codereview.appspot.com/88040049/diff/40001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/88040049/diff/40001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:240: + "shared."); should this section also say ", or server is down at this time". Probably at the beginning of the reasons?
Sign in to reply to this message.
LGTM, wait for PJ's feedback. https://codereview.appspot.com/88040049/diff/40001/src/com/google/enterprise/... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/88040049/diff/40001/src/com/google/enterprise/... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:270: throw new InvalidConfigurationException("The path " + rootPath + " is " This one still feels like an IOException to me. The customer has two options he/she can do to resolve this: 1) update the config as the message says or 2) make the root path unhidden by removing the hidden attribute. So if they make the root path unhidden then they would be required to manually restart the adaptor service. But I can also see that we only suggest updating the config so that's what they most likely do.
Sign in to reply to this message.
> LGTM, wait for PJ's feedback. Already sent question about server down. - technology's compounding interest - On Sat, May 10, 2014 at 9:15 PM, <mifern@google.com> wrote: > LGTM, wait for PJ's feedback. > > > > https://codereview.appspot.com/88040049/diff/40001/src/ > com/google/enterprise/adaptor/fs/FsAdaptor.java > File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): > > https://codereview.appspot.com/88040049/diff/40001/src/ > com/google/enterprise/adaptor/fs/FsAdaptor.java#newcode270 > > src/com/google/enterprise/adaptor/fs/FsAdaptor.java:270: throw new > InvalidConfigurationException("The path " + rootPath + " is " > This one still feels like an IOException to me. The customer has two > options he/she can do to resolve this: 1) update the config as the > message says or 2) make the root path unhidden by removing the hidden > attribute. So if they make the root path unhidden then they would be > required to manually restart the adaptor service. > > But I can also see that we only suggest updating the config so that's > what they most likely do. > > https://codereview.appspot.com/88040049/ >
Sign in to reply to this message.
Change error message.
Sign in to reply to this message.
LGTM. Thank you
Sign in to reply to this message.
Committed 13 May 2014 to Filesystem Adaptor To https://code.google.com/p/plexi.fs/ 3148d05..d8cb37a master -> master
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
