Fix bug# 67471678 - Support If-Modified-Since. by srinicodebytes · Pull Request #115 · googlegsa/database

srinicodebytes · 2017-12-28T02:27:08Z

No description provided.

googlebot · 2017-12-28T02:27:10Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

wiarlawd

Partial review. I haven't looked at the tests yet.

wiarlawd · 2018-01-03T00:25:59Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      log.finer("queried for stream");
      boolean hasAction
          = hasColumn(rs.getMetaData(), GsaSpecialColumns.GSA_ACTION);
+      boolean hasTimestamp =


This is in between getting hasAction and logging hasAction, and we're not logging hasTimestamp. Either get/log/get/log or get/get/log/log.

wiarlawd · 2018-01-03T00:27:29Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+              rs.getTimestamp(GsaSpecialColumns.GSA_TIMESTAMP.toString(),
+                  updateTimestampTimezone);
+          if ((ts != null)
+              && (lastDocIdTimestamp == null || ts.after(lastDocIdTimestamp))) {


What's the merit of checking the cache here, and what's a plausible scenario where ts < lastDocIdTimestamp?

Trying to check if the ts is newer than cached value before updating cache. ts won't be < lastDocIdTimestamp, but can be same as lastDocIdTimestamp. The additional check is not required if I update the cache always..

Removed the check.

wiarlawd · 2018-01-03T00:28:32Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

  private static final HashMap<Integer, String> sqlTypeNames = new HashMap<>();

+  /** Cache for DocId and last modified time stamp. */
+  private static Cache<DocId, Timestamp> docIdLastModifiedMap;


s/docId//; I know it's a map from DocId to Timestamp, but it's really a cache of last modified dates. The "docId" feels like noise. I could see using "Timestamp" consistently, since that's the name of the column, so maybe "lastModifiedCache" or "timestampCache"?

Thx, meant to change the name from "..Map" to "..Cache". Removed "docId" prefix as suggested.

wiarlawd · 2018-01-03T00:30:53Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+      Timestamp lastDocIdTimestamp = docIdLastModifiedMap.getIfPresent(id);
+      if (lastDocIdTimestamp != null
+          && !req.hasChangedSinceLastAccess(
+              new Date(lastDocIdTimestamp.getTime()))) {


You should store Date rather than timestamp, to avoid having to do this for each crawl request. That also avoids the need to check for null here (because hasChangedSinceLastAccess does that, too).

wiarlawd · 2018-01-03T00:32:23Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

        }
+        // Cache last modified time stamp for this DocId
+        if (hasTimestamp) {
+          Timestamp lastDocIdTimestamp = docIdLastModifiedMap.getIfPresent(id);


What does "last" mean here? I think either "cachedTimestamp" or "cachedLastModified".

Renamed to "cachedLastModified"

wiarlawd · 2018-01-03T00:35:49Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

        resp.respondNotFound();
        return;
      }
+      // Check if modified since last access.


This is supposed to happen before the query, or the cache is silly, and we could just compare it to a GSA_TIMESTAMP column in these results (which would be a valid design choice, but the whole idea was to avoid hitting the database in case that's unusually expensive).

Yes, moved it.

wiarlawd · 2018-01-03T01:34:07Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      // One record means one document.
-      resp.setNoFollow(true); 
+      log.log(Level.FINE, "Content modified since last crawl: {0}", id);
+      resp.setNoFollow(true);


Whitespace only change.

Restored the whitespace

wiarlawd · 2018-01-03T01:34:18Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      // In database adaptor's case, we almost never want to follow the URLs.
      // One record means one document.
-      resp.setNoFollow(true); 
+      log.log(Level.FINE, "Content modified since last crawl: {0}", id);


Why not do this right away, on line 724?

Moved to line 724.

wiarlawd · 2018-01-03T02:43:51Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

  private String modeOfOperation;

+  private Calendar updateTimestampTimezone;
+  private DateFormat formatter;


You've got a threading problem here. getDocIds and getModifiedDocIds can run concurrently (though each one separately cannot). We could 1) maintain a separate formatter for each of them (slightly tedious but not a bad option), 2) synchronize the uses, 3) put these into a ThreadLocal (boo), or 4) create an instance for each call (again, slightly tedious, but not a bad option, given that calls happen every 15 minutes at best, by default). I'm leaning toward option 1.

Changed to have separate formatter for each (option 1).
Done.

wiarlawd

More thoughts, still partial.

wiarlawd · 2018-01-03T03:26:00Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

    log.config("primary key: " + uniqueKey);
+
+    docIdLastModifiedMap =
+        CacheBuilder.newBuilder().maximumSize(1000000).build();


We might want to set an initial capacity. The (undocumented) default is 16 (though I think it might be 40 in practice), which is pretty small, and will lead to lots of allocations to grow the cache (done by doubling). Maybe initialCapacity(10000)?

wiarlawd · 2018-01-03T03:26:36Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

    log.config("primary key: " + uniqueKey);
+
+    docIdLastModifiedMap =
+        CacheBuilder.newBuilder().maximumSize(1000000).build();


maximumSize takes a long, so add L to the value (FYI, initialCapacity takes an int).

wiarlawd · 2018-01-03T03:27:24Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

@@ -462,6 +485,8 @@ public void getDocIds(DocIdPusher pusher) throws IOException,
      log.finer("queried for stream");
      boolean hasAction
          = hasColumn(rs.getMetaData(), GsaSpecialColumns.GSA_ACTION);


Should be OK to get the ResultSetMetaData twice, but let's not.

wiarlawd · 2018-01-03T03:28:52Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      log.finer("queried for stream");
      boolean hasAction
          = hasColumn(rs.getMetaData(), GsaSpecialColumns.GSA_ACTION);
+      boolean hasTimestamp =


I thought we were going to clear the cache in getDocIds (but not getModifiedDocIds, obviously). That avoids keeping a record for deleted rows, and in particular, if the ordering in getDocContent is fixed, avoids us reporting deleted rows as unmodified instead.

Yes, missed it. Added a call to invalidateAll().

wiarlawd

I still haven't read the tests, but you changed the semantics of the prod code without changing any tests, which isn't a good sign.

wiarlawd · 2018-01-06T00:48:53Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+    DateFormat formatter = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS Z");
+    formatter.setTimeZone(updateTimestampTimezone.getTimeZone());
    BufferedPusher outstream = new BufferedPusher(pusher);
+    lastModifiedCache.invalidateAll();


I think I would move this to the first line (it's tied to the semantics of getDocIds, not this try statement or the loop, and these fields/variables are used in this order below: lastModifiedCache, formatter, outstream).

wiarlawd · 2018-01-06T00:49:48Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

        // Cache last modified time stamp for this DocId
        if (hasTimestamp) {
-          Timestamp lastDocIdTimestamp = docIdLastModifiedMap.getIfPresent(id);
+          Date cachedLastModified = lastModifiedCache.getIfPresent(id);


This is never used, now, is it? Just overridden below and logged?

Yes, removed call to getIfPresent()

wiarlawd · 2018-01-06T00:55:08Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-            docIdLastModifiedMap.put(id, lastDocIdTimestamp);
-            log.log(Level.FINE, "docIdLastModifiedMap updated: {0}",
-                formatter.format(new Date(lastDocIdTimestamp.getTime())));
+          if ((ts != null)) {


Delete extra parens.

wiarlawd · 2018-01-06T01:00:39Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-                docIdLastModifiedMap.put(id, lastDocIdTimestamp);
-                log.log(Level.FINE, "lastDocIdTimestamp updated: {0}",
-                    formatter.format(new Date(lastDocIdTimestamp.getTime())));
+              if (cachedLastModified == null) {


This if statement is wrong. Why would a row have to be uncached in order to update the cache with a new last modified?

wiarlawd · 2018-01-06T01:01:25Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-                log.log(Level.FINE, "lastDocIdTimestamp updated: {0}",
-                    formatter.format(new Date(lastDocIdTimestamp.getTime())));
+              if (cachedLastModified == null) {
+                cachedLastModified = new Date(ts.getTime());


We're constructing this same Date object twice here, maybe they could be shared, if it doesn't seem too artificial.

wiarlawd · 2018-01-06T01:01:59Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

  @Override
  public void getDocIds(DocIdPusher pusher) throws IOException,
      InterruptedException {
+    DateFormat formatter = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS Z");


This is option 4, not option 1, but that's fine.

srinicodebytes

Code changes were mostly around updating cache. Earlier there were additional checks and cache is updated only if newer, now cache is always updated. Hence the tests didn't change. New tests are mostly around null modified date, modified date is newer or not modified.

srinicodebytes · 2018-01-07T04:34:34Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+    DateFormat formatter = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS Z");
+    formatter.setTimeZone(updateTimestampTimezone.getTimeZone());
    BufferedPusher outstream = new BufferedPusher(pusher);
+    lastModifiedCache.invalidateAll();


srinicodebytes · 2018-01-08T20:37:54Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

        // Cache last modified time stamp for this DocId
        if (hasTimestamp) {
-          Timestamp lastDocIdTimestamp = docIdLastModifiedMap.getIfPresent(id);
+          Date cachedLastModified = lastModifiedCache.getIfPresent(id);


Yes, removed call to getIfPresent()

srinicodebytes · 2018-01-08T22:12:37Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-            docIdLastModifiedMap.put(id, lastDocIdTimestamp);
-            log.log(Level.FINE, "docIdLastModifiedMap updated: {0}",
-                formatter.format(new Date(lastDocIdTimestamp.getTime())));
+          if ((ts != null)) {


srinicodebytes · 2018-01-08T22:32:26Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-                log.log(Level.FINE, "lastDocIdTimestamp updated: {0}",
-                    formatter.format(new Date(lastDocIdTimestamp.getTime())));
+              if (cachedLastModified == null) {
+                cachedLastModified = new Date(ts.getTime());


srinicodebytes · 2018-01-08T22:33:02Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-                docIdLastModifiedMap.put(id, lastDocIdTimestamp);
-                log.log(Level.FINE, "lastDocIdTimestamp updated: {0}",
-                    formatter.format(new Date(lastDocIdTimestamp.getTime())));
+              if (cachedLastModified == null) {


to helper methods in tests.

Fix bug# 67471678 - Support If-Modified-Since.

451f351

srinicodebytes requested a review from wiarlawd December 28, 2017 02:28

wiarlawd suggested changes Jan 3, 2018

View reviewed changes

wiarlawd reviewed Jan 3, 2018

View reviewed changes

Changes as per review.

2a0bd96

wiarlawd reviewed Jan 6, 2018

View reviewed changes

Changes as suggested.

e239eef

srinicodebytes commented Jan 9, 2018

View reviewed changes

Added additional tests. Passed 'content' as argument

836b9c9

to helper methods in tests.

wiarlawd mentioned this pull request Jan 11, 2018

Replace MockRequest with DocRequest. #116

Merged

John Lacey added 2 commits March 1, 2018 16:36

Merge branch 'master' into sri-ifmodifiedsince

e6d0d4c

Replace MockRequest with DocRequest.

ed8ffe7

Conversation

srinicodebytes commented Dec 28, 2017

Uh oh!

googlebot commented Dec 28, 2017

Uh oh!

wiarlawd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiarlawd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiarlawd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!