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

Add support for Search Option "seq_no_primary_term" and Index Options… #2234

Open
wants to merge 4 commits into
base: 8.x
Choose a base branch
from

Conversation

ryu818
Copy link

@ryu818 ryu818 commented Oct 30, 2024

… "if_primary_term" and "if_seq_no"

@ryu818
Copy link
Author

ryu818 commented Oct 30, 2024

This change addresses issue #2232.
Initially, I tried to create options using $endpoint->getParamWhitelist(), but I realized that the options are not only for the endpoint. Therefore, I made changes to add to the existing ones, minimizing the impact.
Please review the changes.

@ruflin
Copy link
Owner

ruflin commented Nov 28, 2024

@ryu818 Really sorry for the late reply. I just started CI on it. Any chance you could add a line to the changelog?

@ryu818
Copy link
Author

ryu818 commented Nov 29, 2024

I have also updated the changelog. However, it seems there are some errors in the CI. I do not have a test execution environment and was unsure how to fix them. Could you please assist me with this?

@ruflin
Copy link
Owner

ruflin commented Nov 29, 2024

Ideally we would have some tests that test the changes. I will try to have a look if the test failures are related to this change or not. My guess it is as we just had recently a clean CI pass. About the changelog, a release went just out a few hours ago so we have a conflict now. Let me know if you want to resolve it otherwise habe to help with it.

For the test suite, do you have phpunit installed? If yes, that is step one. Then to get elasticsearch running, there is a docker compose setup, here some more details how Github runs it: https://github.com/ruflin/Elastica/blob/8.x/.github/workflows/continuous-integration.yaml#L114

@ruflin
Copy link
Owner

ruflin commented Nov 29, 2024

BTW this is the failure I'm worried about:

1) Elastica\Test\IndexTest::testAddDocumentAcrossIndices
Elastic\Elasticsearch\Exception\ClientResponseException: 400 Bad Request: {"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: ifSeqNo is unassigned, but primary term is [1];"}],"type":"action_request_validation_exception","reason":"Validation Failed: 1: ifSeqNo is unassigned, but primary term is [1];"},"status":400}

It seems the injection of ifSeqNo caused an error

ryu818 added a commit to ryu818/Elastica that referenced this pull request Dec 1, 2024
@ryu818
Copy link
Author

ryu818 commented Dec 2, 2024

Thank you for your response.
The conflict has been resolved.
I am running elasticsearch8, php8.1 and phpunit on a development server for another project, not in a docker environment.
I tried to test it with “php vendor/bin/phpunit --filter IndexTest” and so on, but it does not return any response as if there is not enough memory.

1) Elastica\Test\IndexTest::testAddDocumentAcrossIndices
Elastic\Elasticsearch\Exception\ClientResponseException: 400 Bad Request: {"error":{"root_cause":[{"type":"action_request_validation_exception","reason":"Validation Failed: 1: ifSeqNo is unassigned, but primary term is [1];"}],"type":"action_request_validation_exception","reason":"Validation Failed: 1: ifSeqNo is unassigned, but primary term is [1];"},"status":400}

This error occurs when adding a document to Elasticsearch because if_seq_no is not set while if_primary_term is set.
Elasticsearch expects both if_seq_no and if_primary_term to be set.

I think it would be better to fix the part where primary_term is set by default in the document when adding it.
I suspect that if_primary_term is being set despite if_seq_no not being included in the response from Elasticsearch in setVersionParams.

Also, to test if if_seq_no and primary_term are working correctly, it would be sufficient to add a document that was retrieved using getDocument.
(We also have individual setSequenceNumber and setPrimaryTerm methods available, so we could use those as well.)

@ruflin
Copy link
Owner

ruflin commented Dec 6, 2024

The proposed change sounds sensible to me. Do you want to try to implement it that way and add a test?

For the changelog: It seems your editor "adjusted" some of the indentation. Any change you could revert the indentation part to not have a massive diff in the changelog?

@ryu818 ryu818 force-pushed the add/issues2232_seq_no_primary_term branch from f17ace0 to 5337e95 Compare December 9, 2024 01:19
@ryu818
Copy link
Author

ryu818 commented Dec 9, 2024

Regarding the indentation in the ChangeLog, I apologize for the inconvenience.
I have corrected it again, could you please check if it is okay now?

Also, regarding the addition of tests, I apologize for the trouble, but could you please take care of it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants