Lots of good changes in your patch, Varun! I ran all Solr+Solrj+contrib tests and everything passes.
I found a few issues though.
This change is good, but there remains a pre-existing problem:
- int timeout = req.getParams().getInt(BaseSolrResource.UPDATE_TIMEOUT_SECS, -1);
- long startTime = System.nanoTime();
- long endTime = timeout > 0 ? System.nanoTime() + (timeout * 1000 * 1000) : Long.MAX_VALUE;
+ + int timeout = req.getParams().getInt(BaseSolrResource.UPDATE_TIMEOUT_SECS, 60);
+ + long endTime = timeout > 0 ? TimeUnit.NANOSECONDS.convert(timeout, TimeUnit.SECONDS) + System.nanoTime() : Long.MAX_VALUE;
The problem is that System.nanoTime() can return anything in the range [Long.MIN_VALUE, Long.MAX_VALUE] and it intentionally overflows, so making endTime be Long.MAX_VALUE is effectively choosing a random time in the future; this is not the same as "unlimited". This same situation is dealt with in the QueryTimeout implementations - used by ExitableDirectoryReader - by adding a large value to the current value of nanoTime() to arrive at an effectively unlimited end time.
Your changes in ZkSolrResourceLoader.openResource() introduce the possibility of retrying 10 times to fetch a resource from ZK when it's not there (according to zkController.pathExists(file)) - I think in this case the retry loop should be immediately exited, so that the classpath search can start immediately. (I'm assuming that the intent here is not to wait around for another thread/process to finish uploading a resource - if that's the case then the branch for when the path doesn't exist should have a timeout, which it doesn't in your patch.)
Without the changes to SchemaManager the test would fail.
Indeed, when I revert the SchemaManager changes, TestManagedSchemaAPI.testAddFieldAndDocument() fails. But testReloadAndAddSimple() always succeeds, regardless of the SchemaManager changes, so I'm not sure why it's there, since it doesn't have anything to do with new field visibility - can it be removed?