Issue 64060043: FSA FsMonitor does not feed files with hidden ancestors (Closed)
|
Created:
10 years, 2 months ago by Brett Modified:
10 years, 1 month ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionWhen crawling a directory, the FS adaptor
will return the DocIds of hidden files and
folders within that directory. A subsequent
crawl request for a hidden file will return
a 404. A crawl request for a hidden directory
will also return a 404 - importantly its
contents are not fed.
The FS adaptor monitor works differently.
It pays no attention to hidden files or
folders and simply feeds all changed items.
This results in modified files contained
within a hidden directory being fed. My
first inclination was to have the Monitor
not feed any hidden items, but that is not
consistent with the existing lister/retriever
model.
This change has the monitor not feed any
docIds for any items with a hidden ancestor.
The item itself might be hidden and still be
fed. This is consistent with the existing
getDocContent() behavior.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Miguel's Feedback. #MessagesTotal messages: 13
LGTM It would be good for the commit message to say that feeding the hidden directory means that we pick up on the status change from visible -> hidden.
Sign in to reply to this message.
Miguel, how do you feel about this, since you had previously voted against it?
Sign in to reply to this message.
On Feb 14, 2014, at 1:49 PM, ejona@google.com wrote: > LGTM > > It would be good for the commit message to say that feeding the hidden > directory means that we pick up on the status change from visible -> > hidden. > Done. > https://codereview.appspot.com/64060043/
Sign in to reply to this message.
Needs more work. https://codereview.appspot.com/64060043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/64060043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:562: if (!isVisibleDescendantOfRoot(doc.getParent())) { This fails if doc is at the root. Like doc is X:\ or possibly \\host\share (I'm not sure about this one). doc.getParent could return null and a null would cause isVisibleDescendantOfRoot to return false and then skip pushing the doc. There are three signals that we check from the FS for file changes: change, add, delete. For change, the monitor will only get the file that changed. For add, the monitor will get the parent of what was added. For delete, the monitor will get the file and the parent of the file that was deleted. I still don't think we need to make any change to be on parity with getDocContent since for add we will get the parent anyway. A failure point that I see is if the file is visible and then changed to hidden, the adapter will get a change notification and the monitor will get the path of the file. Using isVisibleDescendantOfRoot(doc) causes this file to be skipped, which would not good. Using isVisibleDescendantOfRoot(doc.parent()) would work in that case assuming that doc has a parent.
Sign in to reply to this message.
https://codereview.appspot.com/64060043/diff/1/src/com/google/enterprise/adap... File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right): https://codereview.appspot.com/64060043/diff/1/src/com/google/enterprise/adap... src/com/google/enterprise/adaptor/fs/FsAdaptor.java:562: if (!isVisibleDescendantOfRoot(doc.getParent())) { On 2014/02/15 00:00:20, mifern wrote: > This fails if doc is at the root. Like doc is X:\ or possibly \\host\share (I'm > not sure about this one). doc.getParent could return null and a null would cause > isVisibleDescendantOfRoot to return false and then skip pushing the doc. > > There are three signals that we check from the FS for file changes: change, add, > delete. For change, the monitor will only get the file that changed. For add, > the monitor will get the parent of what was added. For delete, the monitor will > get the file and the parent of the file that was deleted. I still don't think we > need to make any change to be on parity with getDocContent since for add we will > get the parent anyway. > > A failure point that I see is if the file is visible and then changed to hidden, > the adapter will get a change notification and the monitor will get the path of > the file. Using isVisibleDescendantOfRoot(doc) causes this file to be skipped, > which would not good. Using isVisibleDescendantOfRoot(doc.parent()) would work > in that case assuming that doc has a parent. You are correct, I checked that isVisibleDescendantOfRoot() handles a null input, but missed that it returned an undesirable result. I have fixed it.
Sign in to reply to this message.
Miguel's Feedback.
Sign in to reply to this message.
Can this CL get unit tests, including 1 for case Miguel has brought up. Thank you - technology's compounding interest - On Fri, Feb 14, 2014 at 4:11 PM, <Brett.Michael.Johnson@gmail.com> wrote: > Miguel's Feedback. > > http://codereview.appspot.com/64060043/ >
Sign in to reply to this message.
Certainly, Unfortunately, the infrastructure for the tests is in FsAdaptorTest. I will add tests to that for this change. Those tests should fail until this change is incorporated. -- Brett M. Johnson On Feb 14, 2014, at 4:33 PM, PJ <pjo@google.com> wrote: > Can this CL get unit tests, including 1 for case Miguel has brought up. > > Thank you > > > - technology's compounding interest > - > > > On Fri, Feb 14, 2014 at 4:11 PM, <Brett.Michael.Johnson@gmail.com> wrote: > Miguel's Feedback. > > http://codereview.appspot.com/64060043/ >
Sign in to reply to this message.
That doesn't sound right. The tests for this CL should not be in the other CL. Either wait till that one is submitted to add the tests here, or rebase this CL to be ontop of the FsAdaptorTest CL. I'm still personally not convinced that we should have this CL without a comprehensive look at checks in monitor, lister and retriever. Thank you, PJ - technology's compounding interest - On Fri, Feb 14, 2014 at 4:37 PM, Brett Johnson < brett.michael.johnson@gmail.com> wrote: > Certainly, > > Unfortunately, the infrastructure for the tests > is in FsAdaptorTest. I will add tests to that > for this change. Those tests should fail until > this change is incorporated. > > -- > Brett M. Johnson > > > > > On Feb 14, 2014, at 4:33 PM, PJ <pjo@google.com> wrote: > > Can this CL get unit tests, including 1 for case Miguel has brought up. > > Thank you > > > - technology's compounding interest > - > > > On Fri, Feb 14, 2014 at 4:11 PM, <Brett.Michael.Johnson@gmail.com> wrote: > >> Miguel's Feedback. >> >> http://codereview.appspot.com/64060043/ >> > > >
Sign in to reply to this message.
On 2014/02/14 21:49:50, ejona wrote: > Miguel, how do you feel about this, since you had previously voted against it? I still don't see that this is needed mainly because we're going to be throwing away some notifications without really considering all of the corner cases and the alternative is not really a big issue. But if there is an interest in this CL, that is fine with me, I don't see it as a big issue either way, positively or negatively.
Sign in to reply to this message.
On 2014/02/18 22:57:35, mifern wrote: > I still don't see that this is needed mainly because we're going to be throwing > away some notifications without really considering all of the corner cases and > the alternative is not really a big issue. But if there is an interest in this > CL, that is fine with me, I don't see it as a big issue either way, positively > or negatively. That's fair. Nobody has a great example of a common hidden directory/files that would hit this frequently. If we don't have any such common case, then it makes sense to me to drop this CL.
Sign in to reply to this message.
|
