Introduce TransportShardMultiGetFomTranslog by pxsalehi · Pull Request #96492 · elastic/elasticsearch

pxsalehi · 2023-06-01T12:48:39Z

The multi-get counterpart of #95998.

Relates ES-5677

elasticsearchmachine · 2023-06-06T12:26:27Z

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

Relates #96492

…ultiGetFromTranslog

pxsalehi · 2023-06-07T13:02:46Z

#96581 has been merged, and this is a bit smaller now.

kingherc

Some passer-by comments. Will review further.

server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetFomTranslogAction.java

kingherc · 2023-06-07T16:09:02Z

server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetFomTranslogAction.java

+import java.util.Objects;
+
+// TODO(ES-5727): add a retry mechanism to TransportShardMultiGetFromTranslogAction
+public class TransportShardMultiGetFomTranslogAction extends HandledTransportAction<


Where is this class used? I don't see it anywhere apart from tests. Will it be used in the future?

To clarify I see handleGetOnUnpromotableShard() in TransportGetAction, but I do not see a similar one in your PR in TransportMultiGetAction.?

Similarly, it will be used in TransportShardMultiGetAction in a follow-up PR.

OK, thanks for clarifying! Looking forward to the follow-up PR.

kingherc

Did a deeper review today. In general looks good to me, left some comments. My main question still remains, whether you'll including using the new action in this PR or a future PR.

kingherc · 2023-06-08T09:41:05Z

server/src/main/java/org/elasticsearch/action/get/MultiGetShardResponse.java

+        if (this == o) return true;
+        if (o instanceof MultiGetShardResponse == false) return false;
+        MultiGetShardResponse other = (MultiGetShardResponse) o;
+        return Objects.equals(locations, other.locations) && Objects.equals(responses, other.responses);


Is there a reason we do not include the failures variable in equality and hashcode?

Failure includes an Exception, and I think we do not provide logical equality for exceptions. If we include failures, I don't think the serialization tests for objects that have a Failure property would work (since deserializing the same instance would not be equal to the original one because we consider each exception unique). I might be missing something, but I do not think we need to consider failures.

server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetFomTranslogAction.java

kingherc · 2023-06-08T11:34:10Z

server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetFomTranslogAction.java

+import java.util.Objects;
+
+// TODO(ES-5727): add a retry mechanism to TransportShardMultiGetFromTranslogAction
+public class TransportShardMultiGetFomTranslogAction extends HandledTransportAction<


To clarify I see handleGetOnUnpromotableShard() in TransportGetAction, but I do not see a similar one in your PR in TransportMultiGetAction.?

server/src/internalClusterTest/java/org/elasticsearch/get/ShardMultiGetFomTranslogActionIT.java

server/src/test/java/org/elasticsearch/action/get/ShardMultiGetFromTranslogUtil.java

server/src/internalClusterTest/java/org/elasticsearch/get/ShardMultiGetFomTranslogActionIT.java

kingherc · 2023-06-08T12:01:31Z

server/src/internalClusterTest/java/org/elasticsearch/get/ShardMultiGetFomTranslogActionIT.java

+        assertThat(getResponses.size(), equalTo(3));
+        assertNotNull(getResponses.get(0));
+        assertThat(getResponses.get(0).getId(), equalTo("1"));
+        assertThat(getResponses.get(0).getVersion(), equalTo(indexResponse.getVersion()));


I think it might be nice to also assert the returned "field1" value == "value1" here and in all corresponding points below, especially when changed to "value2".

I think last time we agreed that checking version is enough: #95998 (comment)

server/src/internalClusterTest/java/org/elasticsearch/get/ShardMultiGetFomTranslogActionIT.java

kingherc · 2023-06-08T12:09:27Z

...t/java/org/elasticsearch/action/get/ShardMultiGetFromTranslogResponseSerializationTests.java

+    }
+
+    private long randomSegmentGeneration() {
+        return randomBoolean() ? -1L : randomNonNegativeLong();


nit. randomFrom(-1L, randomNonNegativeLong());

feel free to use randomFrom in other places as well where appropriate.

…ultiGetFromTranslog

kingherc

Thanks for taking care of feedback. LGTM. Looking forward to the follow-up PR.

kingherc · 2023-06-09T12:02:46Z

server/src/main/java/org/elasticsearch/action/get/TransportShardMultiGetFomTranslogAction.java

+import java.util.Objects;
+
+// TODO(ES-5727): add a retry mechanism to TransportShardMultiGetFromTranslogAction
+public class TransportShardMultiGetFomTranslogAction extends HandledTransportAction<


OK, thanks for clarifying! Looking forward to the follow-up PR.

pxsalehi · 2023-06-09T14:21:32Z

Thanks, Iraklis!

I've merged this so that I can move on with the rest of the MGet PRs. @tlrx and @henningandersen, if you have anything else on this, please let me know. I'll do it in a follow up.

elasticsearchmachine added the v8.9.0 label Jun 1, 2023

pxsalehi force-pushed the ps230526-TransportMultiGetFromTranslog branch 2 times, most recently from 155310e to 3631dbc Compare June 5, 2023 12:36

pxsalehi added 2 commits June 5, 2023 18:14

Introduce TransportShardMultiGetFomTranslog

1c276dd

Add serialization tests for MultiGetShardRequest

4031e96

pxsalehi force-pushed the ps230526-TransportMultiGetFromTranslog branch from 3631dbc to 4031e96 Compare June 5, 2023 16:14

pxsalehi mentioned this pull request Jun 5, 2023

Add serialization tests for MultiGetShardRequest #96581

Merged

pxsalehi marked this pull request as ready for review June 6, 2023 12:25

pxsalehi added >non-issue :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. labels Jun 6, 2023

pxsalehi requested review from henningandersen and tlrx June 6, 2023 12:26

elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Jun 6, 2023

elasticsearchmachine pushed a commit that referenced this pull request Jun 7, 2023

Add serialization tests for MultiGetShardRequest (#96581)

ecee4e3

Relates #96492

Merge remote-tracking branch 'upstream/main' into ps230526-TransportM…

5524506

…ultiGetFromTranslog

pxsalehi requested review from Tim-Brooks and kingherc June 7, 2023 15:11

kingherc reviewed Jun 7, 2023

View reviewed changes

kingherc reviewed Jun 8, 2023

View reviewed changes

pxsalehi added 2 commits June 9, 2023 13:40

Address review comments

40a3e32

Merge remote-tracking branch 'upstream/main' into ps230526-TransportM…

9fc243e

…ultiGetFromTranslog

pxsalehi requested a review from kingherc June 9, 2023 11:44

kingherc approved these changes Jun 9, 2023

View reviewed changes

pxsalehi merged commit 2c1cd26 into elastic:main Jun 9, 2023

jakelandis mentioned this pull request Jun 9, 2023

ShardMultiGetFomTranslogActionIT.testShardMultiGetFromTranslog routinely fails #96749

Closed

Conversation

pxsalehi commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 6, 2023

Uh oh!

pxsalehi commented Jun 7, 2023

Uh oh!

kingherc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingherc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingherc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pxsalehi commented Jun 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pxsalehi commented Jun 1, 2023 •

edited

Loading