thomaswoeckinger commented on pull request #2605:
URL: https://github.com/apache/lucene-solr/pull/2605#issuecomment-962261641
> Please don't force-push PRs as it inhibits a reviewers ability to see what
was changed.
>
Okay, so before merge as squash is done?
> This time I ran via the CLI instead of IntelliJ and it passed. Maybe my
IntelliJ dependency versions were stale (Caffeine was updated recently) so I
re-generated and still same result with IntelliJ. Weird; is it just me?
>
I use eclipse, and was not able to fail the test.
> I constrained the try-catch around just the call to `toTermSet =
toSearcher.getDocSet(toDeState);` and it passed; I don't get your point Thomas
about resultList.
It is not about the resultList; toTermSet is used in the next few lines and
therefore it must be defined before the try-catch block. Keeping it in the try
block would even allow it to be final (maybe better readability), but anyway
this is a detail discussion, if you prefer it I can change it.
>
> The test adds tons of over 3k docs to Solr one-by-one instead of batching.
Not a deal breaker but sad when I see it. Try this:
>
> ```
> SolrClient client = new EmbeddedSolrServer(h.getCore());
> client.add(persons);
> ```
I will change it.
>
> But hold up...
>
> This logic being added, I noticed, is able to see the filter cache config
without SolrCache having isAsync() and therefore I think we can make this thing
reproduce every time without actually adding any data for a test. See my
related comment in JIRA. Wouldn't that be better? Better to eagerly fail than
to have a failure sneak up on you in production.
Adding isAsync() to the interface is good idea indeed, but will change the
runtime behavior because such queries will always fail, which is currently not
the case.
Anyway we have to made a decision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
