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

Limit RTG exceptions to retry on #105003

Merged

Conversation

pxsalehi
Copy link
Member

@pxsalehi pxsalehi commented Feb 1, 2024

Limit RTG exceptions to retry on and do not throw in getCurrentNodeOfPrimary.
Follow up to #104579 (comment).

Relates ES-5727

@pxsalehi pxsalehi added >non-issue :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. test-update-serverless labels Feb 1, 2024
@elasticsearchmachine elasticsearchmachine added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.13.0 labels Feb 1, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@pxsalehi
Copy link
Member Author

pxsalehi commented Feb 1, 2024

I think i will also open a test PR to show that realtime-(m)get on unassigned shard will/should fail, to have that behaviour documented. But merging this first, would help with the mget PRs too. Sorry the whole get/mget stateless/stateful dimension results in too many PRs.

@pxsalehi
Copy link
Member Author

pxsalehi commented Feb 1, 2024

serverless check failure is not related and has an open issue.

@henningandersen
Copy link
Contributor

Can we add a test that fails without this? I think a test that stops the node containing the shard could do so. Fine if that is in stateless only, but did not see a PR there.

@henningandersen
Copy link
Contributor

I think i will also open a test PR to show that realtime-(m)get on unassigned shard will/should fail, to have that behaviour documented. But merging this first, would help with the mget PRs too. Sorry the whole get/mget stateless/stateful dimension results in too many PRs.

Sorry, missed this, I could be ok with that, but then we need to validate when writing the unassigned test that it failed with the original version of this. Feels a bit backwards and I would prefer to have the failing test first if possible, but can be persuaded 🙂

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pxsalehi pxsalehi added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Feb 1, 2024
@pxsalehi
Copy link
Member Author

pxsalehi commented Feb 1, 2024

Feels a bit backwards

It is a bit! :) I'm just trying to reduce all the pending PRs. I can open the PR first if you want. I have a local version of it. It takes me a bit to polish it.

@pxsalehi
Copy link
Member Author

pxsalehi commented Feb 1, 2024

ok. thanks to @ywangd 's quick reviews and help, I've been unblocked differently by merging the mget PRs. This can then wait for the test PR first.

@pxsalehi
Copy link
Member Author

pxsalehi commented Feb 1, 2024

failure was #103588

@pxsalehi pxsalehi added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 1, 2024
@elasticsearchmachine elasticsearchmachine merged commit e2d68ae into elastic:main Feb 1, 2024
14 checks passed
@pxsalehi pxsalehi deleted the ps240201-RTG-retry-refine branch February 1, 2024 13:37
@pxsalehi
Copy link
Member Author

pxsalehi commented Feb 1, 2024

Oh! I didn't want to merge this actually. My bad! 🤦🏼
I think there are more ways that getCurrentNodeOfPrimary could throw and I didn't cover all of them in the PR. I'll open yet another follow up PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants