FS Fix b/27579240: Feed large directory listings asynchronously to avoid timeouts.

This change returns at most maxHtmlSize links in the generated HTML
of a directory listing. If the directory has more than maxHtmlSize
entries, the directory stream is closed and the generated HTML is
returned as the response. A separate thread will then be launched
to push the directory entries via the DocIdPusher. This should avoid
getDocContent timeouts reading large directories.

[Brett, 2017-01-10 08:13:03]

Clean up tests

[JohnL, 2017-01-11 00:09:20]

Minor suggestions and questions.

John L

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

https://codereview.appspot.com/316140043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1096: DocId docId =
delegate.newDocId(file);
Why did you remove the try statement here?

https://codereview.appspot.com/316140043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1101: log.log(Level.FINE,
"Directory listing for {0} exceeds maxHtmlSize "
I thought you were going to include something in the HTML as well here.

https://codereview.appspot.com/316140043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1115: private class
AsyncDirectoryContentPusher extends Thread {
I think Runnable is generally preferred when you don't need to subclass Thread.
Also, why not use a newFixedThreadPool executor?

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

https://codereview.appspot.com/316140043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:1242: expected =
Lists.transform(Lists.newArrayList(files),
I think the use of transform here, where it isn't needed, is a little obfuscated
compared to the simple loop. Especially since you can elide the try statement
with the loop, so it's half as many lines.

[Brett, 2017-01-11 06:07:01]

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

https://codereview.appspot.com/316140043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1096: DocId docId =
delegate.newDocId(file);
On 2017/01/11 00:09:20, JohnL wrote:
> Why did you remove the try statement here?

After long pathname support was added, newDocId() no longer throws
IllegalArgumentException.

https://codereview.appspot.com/316140043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1101: log.log(Level.FINE,
"Directory listing for {0} exceeds maxHtmlSize "
On 2017/01/11 00:09:20, JohnL wrote:
> I thought you were going to include something in the HTML as well here.

I could do that. It would require a small mod to HtmlResponseWriter,
which doesn't support writing arbitrary text.

[Brett, 2017-01-11 23:05:20]

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

https://codereview.appspot.com/316140043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1101: log.log(Level.FINE,
"Directory listing for {0} exceeds maxHtmlSize "
On 2017/01/11 06:07:00, Brett wrote:
> On 2017/01/11 00:09:20, JohnL wrote:
> > I thought you were going to include something in the HTML as well here.
> 
> I could do that. It would require a small mod to HtmlResponseWriter,
> which doesn't support writing arbitrary text.

Done.

https://codereview.appspot.com/316140043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1115: private class
AsyncDirectoryContentPusher extends Thread {
On 2017/01/11 00:09:20, JohnL wrote:
> I think Runnable is generally preferred when you don't need to subclass
Thread.
> Also, why not use a newFixedThreadPool executor?

Done.

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

https://codereview.appspot.com/316140043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:1242: expected =
Lists.transform(Lists.newArrayList(files),
On 2017/01/11 00:09:20, JohnL wrote:
> I think the use of transform here, where it isn't needed, is a little
obfuscated
> compared to the simple loop. Especially since you can elide the try statement
> with the loop, so it's half as many lines.

Done.

[Brett, 2017-01-11 23:06:14]

Review Feedback

[JohnL, 2017-01-12 22:17:47]

Minor comments.

John L

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

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1115: "Directory listing for
{0} exceeds maxHtmlSize of {1}. "
You're going to get localized numbers here, e.g., "1,000". Seems OK to me, just
thought I'd mention it.

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1117: doc, new
Integer(maxHtmlLinks));
Use auto-boxing? s/new Integer(...)/.../

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right):

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:123: if (htmlText
== null) {
Does this just avoid the NPE from write, or prevent writing "null"? Is either
really worth it?

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:249: public String
escapeContent(String raw) {
Why not include the escaping in addHtml, as we do for addLink and addAnchor
labels?

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:253: public String
escapeAttributeValue(String raw) {
Public just for consistency?

https://codereview.appspot.com/316140043/diff/40001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right):

https://codereview.appspot.com/316140043/diff/40001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:1240:
ImmutableList.Builder<Record> builder = ImmutableList.builder();
Also check for the overflow message in html here?

[Brett, 2017-01-12 23:02:52]

Review Feedback

[Brett, 2017-01-12 23:03:13]

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

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1115: "Directory listing for
{0} exceeds maxHtmlSize of {1}. "
On 2017/01/12 22:17:47, JohnL wrote:
> You're going to get localized numbers here, e.g., "1,000". Seems OK to me,
just
> thought I'd mention it.

Done.

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/FsAdaptor.java:1117: doc, new
Integer(maxHtmlLinks));
On 2017/01/12 22:17:47, JohnL wrote:
> Use auto-boxing? s/new Integer(...)/.../

Done.

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
File src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java (right):

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:123: if (htmlText
== null) {
On 2017/01/12 22:17:47, JohnL wrote:
> Does this just avoid the NPE from write, or prevent writing "null"? Is either
> really worth it?

It was just copy-n-paste from addLink above.

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:249: public String
escapeContent(String raw) {
On 2017/01/12 22:17:47, JohnL wrote:
> Why not include the escaping in addHtml, as we do for addLink and addAnchor
> labels?

To avoid escaping actual HTML tags.

https://codereview.appspot.com/316140043/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/fs/HtmlResponseWriter.java:253: public String
escapeAttributeValue(String raw) {
On 2017/01/12 22:17:47, JohnL wrote:
> Public just for consistency?

yes.

[Brett, 2017-01-12 23:15:56]

Review Feedback

[Brett, 2017-01-12 23:16:17]

https://codereview.appspot.com/316140043/diff/40001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java (right):

https://codereview.appspot.com/316140043/diff/40001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/fs/FsAdaptorTest.java:1240:
ImmutableList.Builder<Record> builder = ImmutableList.builder();
On 2017/01/12 22:17:47, JohnL wrote:
> Also check for the overflow message in html here?

Done.

[JohnL, 2017-01-12 23:27:22]

LGTM

John L

[Brett, 2017-01-12 23:30:24]

Committed 12 January 2107 to Filesystem Adaptor:
https://github.com/googlegsa/filesystem/commit/d654ca5c28872e44fe78247ba3fa86...