Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[QUESTION] Consider reverting RefreshPolicy to HLRC #52

Closed
tlfeng opened this issue Feb 5, 2021 · 2 comments
Closed

[QUESTION] Consider reverting RefreshPolicy to HLRC #52

tlfeng opened this issue Feb 5, 2021 · 2 comments
Labels
>FORK Related to the fork process question Questions about how things work, requests for help :sanitize Removing elastic specific artifacts

Comments

@tlfeng
Copy link
Collaborator

tlfeng commented Feb 5, 2021

All the usage of withRefresh() has been removed in the commit 4d4f198 (remove all trace of x-pack security), and the code used for parsing the refresh parameter in High Level Rest Client. Probably we could bring it back.

Reason
Also written in my comment 4d4f198#r46835616.

  1. "refresh" parameter is used in Get and Get Source API, etc.

  2. The related code RefreshPolicy exists outside "x-pack" folder. Although Elasticsearch added a client side version of enum RefreshPolicy in Hight-Level-Rest-Client under x-pack "security" package (by commit elastic/elasticsearch@f063587 ), it is nearly a copy of the original server side enum WriteRequest.RefreshPolicy. The server side RefreshPolicy still valid.

Affected test:

  1. 4 test cases in RequestConvertersTests in PR [TEST] fix RequestConvertersTests failure #42
  2. UpdateByQueryIT.testUpdateByQuery
REPRODUCE WITH: ./gradlew ':client:rest-high-level:asyncIntegTest' --tests "org.elasticsearch.client.UpdateByQueryIT.testUpdateByQuery" -Dtests.seed=7C260435FE74FF9A -Dtests.security.manager=true -Dtests.locale=sk-SK -Dtests.timezone=America/Kentucky/Monticello -Druntime.java=15

org.elasticsearch.client.UpdateByQueryIT > testUpdateByQuery FAILED
    java.lang.AssertionError: expected:<2> but was:<1>
        at __randomizedtesting.SeedInfo.seed([7C260435FE74FF9A:867672C55A50F0DC]:0)
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:645)
        at org.junit.Assert.assertEquals(Assert.java:631)
        at org.elasticsearch.client.UpdateByQueryIT.testUpdateByQuery(UpdateByQueryIT.java:105)
  1. ReindexIT.testDeleteByQuery
REPRODUCE WITH: ./gradlew ':client:rest-high-level:asyncIntegTest' --tests "org.elasticsearch.client.ReindexIT.testDeleteByQuery" -Dtests.seed=7C260435FE74FF9A -Dtests.security.manager=true -Dtests.locale=no -Dtests.timezone=MST7MDT -Druntime.java=15

org.elasticsearch.client.ReindexIT > testDeleteByQuery FAILED
    java.lang.AssertionError: expected:<2> but was:<3>
        at __randomizedtesting.SeedInfo.seed([7C260435FE74FF9A:72E9011EA7BB800C]:0)
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:645)
        at org.junit.Assert.assertEquals(Assert.java:631)
        at org.elasticsearch.client.ReindexIT.testDeleteByQuery(ReindexIT.java:225)
  1. CRUDDocumentationIT.testGetSource
REPRODUCE WITH: ./gradlew ':client:rest-high-level:asyncIntegTest' --tests "org.elasticsearch.client.documentation.CRUDDocumentationIT.testGetSource" -Dtests.seed=6920D29241A2C6F7 -Dtests.security.manager=true -Dtests.locale=mt-MT -Dtests.timezone=Pacific/Norfolk -Druntime.java=15

org.elasticsearch.client.documentation.CRUDDocumentationIT > testGetSource FAILED
    ElasticsearchStatusException[Elasticsearch exception [type=resource_not_found_exception, reason=Document not found [posts]/[_doc]/[1]]]
        at __randomizedtesting.SeedInfo.seed([6920D29241A2C6F7:F329DA9BCC2B1A30]:0)
        at org.elasticsearch.rest.BytesRestResponse.errorFromXContent(BytesRestResponse.java:187)SearchDocumentationIT
        at org.elasticsearch.client.RestHighLevelClient.parseEntity(RestHighLevelClient.java:1735)
        at org.elasticsearch.client.RestHighLevelClient.parseResponseException(RestHighLevelClient.java:1712)
        at org.elasticsearch.client.RestHighLevelClient.internalPerformRequest(RestHighLevelClient.java:1469)
        at org.elasticsearch.client.RestHighLevelClient.performRequest(RestHighLevelClient.java:1441)
        at org.elasticsearch.client.RestHighLevelClient.performRequestAndParseEntity(RestHighLevelClient.java:1408)
        at org.elasticsearch.client.RestHighLevelClient.getSource(RestHighLevelClient.java:786)
        at org.elasticsearch.client.documentation.CRUDDocumentationIT.testGetSource(CRUDDocumentationIT.java:1458)
@tlfeng tlfeng added question Questions about how things work, requests for help :sanitize Removing elastic specific artifacts >FORK Related to the fork process labels Feb 5, 2021
@tlfeng tlfeng changed the title Considering reverting RefreshPolicy to High-Level-Rest-Client [QUESTION] Considering reverting RefreshPolicy to HLRC Feb 5, 2021
@tlfeng tlfeng changed the title [QUESTION] Considering reverting RefreshPolicy to HLRC [QUESTION] Consider reverting RefreshPolicy to HLRC Feb 5, 2021
tlfeng referenced this issue Feb 7, 2021
This commit removes all trace of the security high level rest client and other reference to x-pack security

Co-authored-by: Rabi Panda <[email protected]>
@tlfeng
Copy link
Collaborator Author

tlfeng commented Feb 7, 2021

Inspired by @harold-wang in above commit ^, I checked the usage of "refresh" parameter.

"refresh" parameter is widely used in REST APIs, and "Java High Level REST Client APIs": Get Source, ReIndex, Delete By Query, Update By Query

I suggest we revert the commit elastic/elasticsearch@f063587, and bring the codes related to "RefreshPolicy" back.

@tlfeng
Copy link
Collaborator Author

tlfeng commented Feb 8, 2021

Resolved by above PR (# 55)

@tlfeng tlfeng closed this as completed Feb 8, 2021
ritty27 pushed a commit to ritty27/OpenSearch that referenced this issue May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>FORK Related to the fork process question Questions about how things work, requests for help :sanitize Removing elastic specific artifacts
Projects
None yet
Development

No branches or pull requests

1 participant