Fix for ignoring name parameter for Percolator queries #40843
Conversation
There was a problem hiding this comment.
Thanks @gurkankaymak , I left some comments. Can you also add a test to ensure that the fix is correct ?
modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java
Outdated
Show resolved
Hide resolved
modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java
Outdated
Show resolved
Hide resolved
modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java
Outdated
Show resolved
Hide resolved
| @@ -555,7 +559,13 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) { | |||
| listener.onResponse(null); | |||
| }, listener::onFailure)); | |||
| }); | |||
| return new PercolateQueryBuilder(field, documentType, documentSupplier::get); | |||
|
|
|||
sure, I just left, I'll add a test when I get back on my computer |
colings86
added
the
:Search/Percolator
|
Pinging @elastic/es-search |
|
hi @jimczi i've returned from the military service :) |
|
test this please |
modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @gurkankaymak! Can you merge with master and we'll check that the tests pass in CI ?
|
Hi @gurkankaymak, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile? |
|
thanks for review @jimczi I synced branch with the master, can you start the CI tests |
Sure I will but did you see #40843 (comment) ? You need to add your e-mail into your github profile or use the same e-mail than the one that is already present. |
|
the email is the same as before and exists in my profile, I pushed the sync code from different computer and i think i used the github username part instead of the full email, not sure how to fix this |
Can you try to rebase to keep your first commit only (which contains the correct username) ? |
ShardId already implements Writeable so there is no need for it to implement Streamable too. Also the readShardId static method can be easily replaced with direct usages of the constructor that takes a StreamInput as argument.
This commit removes manual parsing of JVM options when calculating ergonomics. This is to avoid a situation that we parse values differently than the JVM would. In fact, we already have a bug along these lines today. It is possible to start the JVM with the same flag multiple times on the command line. In this case, the last value wins. For example, -Xmx1g -Xmx2g would start the JVM with a heap size of two gigabytes. Our JVM ergonomics ignores this possibility and instead the first value is winning! Our strategy to avoid manual parsing of the JVM options is to start the Java command line parser (without actually starting a JVM) by invoking java with the same command line flags as presented and request that the JVM tell us what values it would start with. This ensures that we have the correct values when making ergonomic decisions. Moreover, our strategy also is ignoring ES_JAVA_OPTS which could override the heap size as well leading to incorrect ergonomic choices. This commit address this issue too.
This processor uses the lucene HTMLStripCharFilter class to remove HTML entities from a field. This adds to the char filter, so that there is possibility to store the stripped version as well. Note, that the characeter filter replaces tags with a newline, so that the produced HTML will look slightly different than the incoming HTML with regards to newlines.
This commit makes AnalyzeResponse and its various helper classes implement Writeable. The classes are also now immutable. Relates to elastic#34389
…se (elastic#41995) and use Version.CURRENT instead of passing down index created version
If the max doc in the index is greater than the minimum total term frequency among the requested fields we need to adjust max doc to be equal to the min ttf. This was removed by mistake when fixing elastic#41125. Closes elastic#41934
This test was added while a PR removing transportClientRatio was in flight.
…ove interval (elastic#42427) * add support for fixed_interval, calendar_interval, remove interval * adapt HLRC * checkstyle * add a hlrc to server test * adapt yml test * improve naming and doc * improve interface and add test code for hlrc to server * address review comments * repair merge conflict * fix date patterns * address review comments * remove assert for warning * improve exception message * use constants
…#42506) This change fixes a race condition that would result in an in-memory data structure becoming out-of-sync with persistent tasks in cluster state. If repeated often enough this could result in it being impossible to open any ML jobs on the affected node, as the master node would think the node had capacity to open another job but the chosen node would error during the open sequence due to its in-memory data structure being full. The race could be triggered by opening a job and then closing it a tiny fraction of a second later. It is unlikely a user of the UI could open and close the job that fast, but a script or program calling the REST API could. The nasty thing is, from the externally observable states and stats everything would appear to be fine - the fast open then close sequence would appear to leave the job in the closed state. It's only later that the leftovers in the in-memory data structure might build up and cause a problem.
…astic#42534) Using map and filter avoids the garbage from all the Stream.of calls that flatMap necessitated. Performance is better when there are masses of fields.
This commit moves the expensive configuration-time calculation of Java runtime version information to runtime instead and also makes that work cacheable. This roughly equates to about a 50% reduction in project configuration time.
SplitIndexIT#testSplitIndexPrimaryTerm sometimes timeout due to relocating many shards. This change adjusts loads and increases the timeout.
* Address test failures for SmokeTestWatcherWithSecurityIT There are likely multiple root causes to the seemingly random failures generated by SmokeTestWatcherWithSecurityIT. This commit un-mutes this this test, address one known cause and adds debug logging for this test. The known root cause for one failure is that we can have a Watch running that is reading data from an index. Before we stop Watcher we delete that index. If Watcher happens to execute after deletion of the index but before the stop of Watcher the test can fail. The fix here is to simply move the index deletion after the stop of Watcher. Related elastic#35361 Related elastic#30777 Related elastic#33291 Related elastic#29893
This change verifies and aborts recovery if source and target have the same syncId but different sequenceId. This commit also adds an upgrade test to ensure that we always utilize syncId.
This commit removes the act of renewing some retention leases during a retention lease recovery test. Having renewal does not add anything extra to this test, but does allow for some situations where the test can fail spuriously (i.e., in a way that does not indicate that production code is broken).
Relates elastic#39687
* This call can fail when it tries to re-schedule the timeout check after the threadpool was shut down already failing tests with RejectedExecutionException
We need more information to understand why CcrRetentionLeaseIT is failing. This commit adds some debug log to retention leases and enables them in CcrRetentionLeaseIT.
…ies' into FixIgnoringNameForPercolatorQueries
|
@jimczi I'm new to rebase and i think i did not use it correctly, currently i don't have much time to read and inspect to fix it I think it will be much easier to create a new pull request from a separate branch since my changes were simple, is it ok for you? |
Fine with me, thanks |
|
Superseded by #42598 |
fixes #40405