change monitor fan out logic to handle empty request and checking node attributes before fanning out #1815
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e attributes before fanning out Signed-off-by: Surya Sashank Nistala <[email protected]>
eirsep
requested review from
lezzago,
AWSHurneyt,
sbcd90,
getsaurabh02,
praveensameneni,
bowenlan-amzn,
rishabhmaurya,
engechas,
riysaxen-amzn,
jowg-amazon,
amsiglan and
goyamegh
as code owners
engechas
reviewed
Mar 24, 2025
| if (candidateNodeAttributes.containsKey("di_number") && candidateNodeAttributes["di_number"] != null && | ||
| coordinatorNodeAttributes.containsKey("di_number") && candidateNodeAttributes["di_number"] != null | ||
| ) { | ||
| return candidateNodeAttributes["di_number"] as Int >= coordinatorNodeAttributes["di_number"] as Int |
There was a problem hiding this comment.
Is there a downside to keeping the fanout fully within the same DI (== instead of >=)? That seems like a safer approach than relying on backwards compatibility in newer DIs
| @@ -207,6 +207,22 @@ class TransportDocLevelMonitorFanOutAction | |||
| listener: ActionListener<DocLevelMonitorFanOutResponse> | |||
| ) { | |||
| try { | |||
| // handle situations where coordinator node is not on the same opensearch version or | |||
| // from the same domain instance and return no-op response instead of failing entire fan-out | |||
| if (request.indexExecutionContext == null) { | |||
There was a problem hiding this comment.
Checking this specific field for null seems brittle. Is there a more definitive way to see if the deserialization failed?
AWSHurneyt
reviewed
Mar 24, 2025
Comment on lines
+17
to
+18
| if (candidateNodeAttributes.containsKey("di_number") && candidateNodeAttributes["di_number"] != null && | ||
| coordinatorNodeAttributes.containsKey("di_number") && candidateNodeAttributes["di_number"] != null |
There was a problem hiding this comment.
Not blocking:
I think the use of containsKey and != null would be redundant in this context.
| if (candidateNodeAttributes.containsKey("di_number") && candidateNodeAttributes["di_number"] != null && | ||
| coordinatorNodeAttributes.containsKey("di_number") && candidateNodeAttributes["di_number"] != null | ||
| ) { | ||
| return candidateNodeAttributes["di_number"] as Int >= coordinatorNodeAttributes["di_number"] as Int |
There was a problem hiding this comment.
Would we want this to instead be an equalTo operator, or are we confident that the coodinator node will always be in the old di?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
change monitor fan out logic to handle empty request and checking node attributes before fanning out
this handles upgrade situation where old version is fanning out to new version and encounters failure in request serialization to silently ignore processing the set of shards assigned