FSA Add Config Options to Expire Content Based on LastAccess/LastModified Time

This change adds configuration options to expire content based on
    LastAccessTime and/or LastModifiedTime. Each allows two different
    forms of specification:
      - Expire content older than some specific date.  (Absolute)
      - Expire content older than some number of days. (Relative)
    
    The configuration parameters are:
    filesystemadaptor.lastAccessedDate - Absolute last accessed time expiration
    filesystemadaptor.lastAccessedDays - Relative last accessed time expiration
    filesystemadaptor.lastModifiedDate - Absolute last modified time expiration
    filesystemadaptor.lastModifiedDays - Relative last modified time expiration
    
    Values for the Relative forms of configuration are positive integer
    number of days.  For instance:
       filesystemadaptor.lastAccessedDays=365
    would expire content that had not been accessed for a year.
    
    Values for the Absolute forms of configuration are ISO8601 date
    only format strings: yyyy-MM-dd.  For instance:
       filesystemadaptor.lastModifiedDate=2010-06-01
    would expire content that had not been modified since the beginning
    of June 2010.

[pjo, 2014-05-14 00:24:00]

Miguel and Marc,

Please both review this CL.

Brett, thanks for the detailed description.

Thank you,
PJ

[Brett, 2014-05-14 21:44:46]

Update overview documentation.

[mifern, 2014-05-14 21:49:21]

Did you take incremental updates into consideration?

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:314: 
If the date is in the future nothing will be crawled so in that case should we
display an error message that the date is in the future?

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:347: return new
ExpiringFileTimeFilter(Integer.parseInt(configDays));
configDays could be a negative value or 0. Both of these should probably not be
allowed?

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:359:
ISO8601_DATE_FORMAT.parse(configDate).getTime()));
Future date?

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:486: new Object[] {doc,
lastAccessTime.toString().substring(0, 10)});
Wouldn't localization impact lastAccessTime.toString().substring(0, 10)? Just
lastAccessTime should be enough.

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:493:
attrs.lastModifiedTime().toString().substring(0, 10)});
Same as above.

[myk, 2014-05-14 22:06:03]

LGTM -- only have one question (which occurs twice) about duplicate comments.

https://codereview.appspot.com/96330043/diff/20001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/96330043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:101: /** The config
parameter name for earliest last accessed time allowed. */
this comment matches the one above it; should one say "Relative" and one
"Absolute" ?

https://codereview.appspot.com/96330043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:109: /** The config
parameter name for earliest last modified time allowed. */
same here.

[Brett, 2014-05-14 22:06:22]

On 2014/05/14 21:49:21, mifern wrote:
> Did you take incremental updates into consideration?

All filtering is done at crawl time.

[Brett, 2014-05-14 22:11:50]

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:314: 
On 2014/05/14 21:49:21, mifern wrote:
> If the date is in the future nothing will be crawled so in that case should we
> display an error message that the date is in the future?

Should I log a warning and leave it, or should it be a configuration error?

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:347: return new
ExpiringFileTimeFilter(Integer.parseInt(configDays));
On 2014/05/14 21:49:21, mifern wrote:
> configDays could be a negative value or 0. Both of these should probably not
be
> allowed?

Values <= 0 will throw IllegalArgumentException out of the filter constructor,
which gets rethrown as InvalidConfigurationException 5 lines below.

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:486: new Object[] {doc,
lastAccessTime.toString().substring(0, 10)});
On 2014/05/14 21:49:21, mifern wrote:
> Wouldn't localization impact lastAccessTime.toString().substring(0, 10)? Just
> lastAccessTime should be enough.

No.  FileTime.toString() returns the time in ISO8601 format:
YYYY-MM-DDThh:mm:ss[.s+]Z

https://codereview.appspot.com/96330043/diff/20001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/96330043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:101: /** The config
parameter name for earliest last accessed time allowed. */
On 2014/05/14 22:06:03, myk wrote:
> this comment matches the one above it; should one say "Relative" and one
> "Absolute" ?

Done.  And kept it on one line!

https://codereview.appspot.com/96330043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:109: /** The config
parameter name for earliest last modified time allowed. */
On 2014/05/14 22:06:03, myk wrote:
> same here.

Done.

https://codereview.appspot.com/96330043/diff/20001/test/com/google/enterprise...
File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right):

https://codereview.appspot.com/96330043/diff/20001/test/com/google/enterprise...
test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:1008: public void
testAdaptorInitInvalidLastAccessDaysNonNumeric() throws Exception {
I already fixed all these over 80 lines.

[Brett, 2014-05-14 22:13:47]

Feedback and fixed long lines in the tests.

[myk, 2014-05-14 22:16:03]

Thank you.  (Still) LGTM.

https://codereview.appspot.com/96330043/diff/20001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/96330043/diff/20001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:101: /** The config
parameter name for earliest last accessed time allowed. */
On 2014/05/14 22:11:50, Brett wrote:
> On 2014/05/14 22:06:03, myk wrote:
> > this comment matches the one above it; should one say "Relative" and one
> > "Absolute" ?
> 
> Done.  And kept it on one line!

Nice!

[mifern, 2014-05-14 22:18:06]

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/96330043/diff/1/src/com/google/enterprise/adap...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:314: 
I would lean towards a configuration error.

On 2014/05/14 22:11:50, Brett wrote:
> On 2014/05/14 21:49:21, mifern wrote:
> > If the date is in the future nothing will be crawled so in that case should
we
> > display an error message that the date is in the future?
> 
> Should I log a warning and leave it, or should it be a configuration error?

[Brett, 2014-05-14 22:45:29]

No future termination dates, John Connor.

[pjo, 2014-05-15 19:09:39]

Thank you.  LGTM.

[mifern, 2014-05-15 21:18:10]

https://codereview.appspot.com/96330043/diff/60001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/96330043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:115: new
SimpleDateFormat("yyyy-MM-dd");
We already had a SimpleDateFormat below. Can we use that instead? Also, Eric
suggested using ThreadLocal<SimpleDateFormat> instead of just SimpleDateFormat
he had a reason for that, that I can't remember at the moment.

[JohnL, 2014-05-15 21:24:47]

https://codereview.appspot.com/96330043/diff/60001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/96330043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:115: new
SimpleDateFormat("yyyy-MM-dd");
On 2014/05/15 21:18:09, mifern wrote:
> We already had a SimpleDateFormat below. Can we use that instead? Also, Eric
> suggested using ThreadLocal<SimpleDateFormat> instead of just SimpleDateFormat
> he had a reason for that, that I can't remember at the moment.

SimpleDateFormat is not thread-safe, so access must be synchronized or
restricted to a single thread.

[pjo, 2014-05-15 21:46:11]

Thank you

https://codereview.appspot.com/96330043/diff/60001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/96330043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:115: new
SimpleDateFormat("yyyy-MM-dd");
On 2014/05/15 21:24:47, John L wrote:
> On 2014/05/15 21:18:09, mifern wrote:
> > We already had a SimpleDateFormat below. Can we use that instead? Also, Eric
> > suggested using ThreadLocal<SimpleDateFormat> instead of just
SimpleDateFormat
> > he had a reason for that, that I can't remember at the moment.
> 
> SimpleDateFormat is not thread-safe, so access must be synchronized or
> restricted to a single thread.


Make this instance live inside of method 
it is used in.

No need for it to be instance or class variable.

Thank you

[Brett, 2014-05-16 00:47:55]

Move SimpleDateFormat to a local variable to avoid thread safety issues.

[Brett, 2014-05-16 00:56:01]

Committed 15 May 2014 to Filesystem Adaptor:
To https://code.google.com/p/plexi.fs/
   cc553af..93cb90d  master -> master

https://codereview.appspot.com/96330043/diff/60001/src/com/google/enterprise/...
File src/com/google/enterprise/adaptor/fs/FsAdaptor.java (right):

https://codereview.appspot.com/96330043/diff/60001/src/com/google/enterprise/...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:115: new
SimpleDateFormat("yyyy-MM-dd");
On 2014/05/15 21:46:11, pjo wrote:
> On 2014/05/15 21:24:47, John L wrote:
> > On 2014/05/15 21:18:09, mifern wrote:
> > > We already had a SimpleDateFormat below. Can we use that instead? Also,
Eric
> > > suggested using ThreadLocal<SimpleDateFormat> instead of just
> SimpleDateFormat
> > > he had a reason for that, that I can't remember at the moment.
> > 
> > SimpleDateFormat is not thread-safe, so access must be synchronized or
> > restricted to a single thread.
> 
> 
> Make this instance live inside of method 
> it is used in.
> 
> No need for it to be instance or class variable.
> 
> Thank you

Done.