HDDS-11013. Ensure version is always set in ContainerCommandRequestProto #6812

swamirishi · 2024-06-13T20:19:55Z

What changes were proposed in this pull request?

Setting ClientVersion in the ContainerCommandRequestProto object in XceiverGrpcClient send method, to ensure client version is always set on the request object.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-11013

How was this patch tested?

Existing Unit test, no functional change.

…ion in one centralised function Change-Id: I540e518819db3ebfec1aca5b3a31d1b5ba5fee41

Change-Id: Id5abeebc078d39d15d516cf804dcdaaa3ed2467c Change-Id: Ic5a87d9e696a462cfe92ff35a0068dfeba97541c

hemantk-12

Thanks @swamirishi for the patch. In general, change looks good to me. Left a comment about one change in ClientVersion.

nit: I don't think it is a good idea to provide a helper function on top of builder. I guess you have done it this way to make the version mandatory field and ContainerCommandRequestProto.newBuilder() is generated by protobuf.

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/ClientVersion.java

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java

Change-Id: I8936e48570d75052a32c40a9aee751628b753585

swamirishi · 2024-06-14T01:03:52Z

Thanks @swamirishi for the patch. In general, change looks good to me. Left a comment about one change in ClientVersion.

nit: I don't think it is a good idea to provide a helper function on top of builder. I guess you have done it this way to make the version mandatory field and ContainerCommandRequestProto.newBuilder() is generated by protobuf.

Yeah I wanted to mandate client versions are being set to the latest value on all the request. I don't think there is a programatic way of doing this in protobuf. From what I know we can only set a constant value in proto file. Moreover server side and client side handling is going to be different in this patch. #6779

swamirishi · 2024-06-14T01:06:59Z

We can deprecate this method call, when we would stop supporting older clients on the server side.

hemantk-12

LGTM+1.

adoroszlai

Thanks @swamirishi for extracting this.

There are two leftover calls to newBuilder in ContainerProtocolCalls (echo and readContainerFromAllNodes) and two in TestHddsDispatcher (newPutSmallFile and getWriteChunkRequest0).

adoroszlai

Thanks again @swamirishi for the patch.

One question: instead of replacing all the calls, how about setting version in sendCommand(Async)? That would simplify this change a whole lot. It would also avoid potential problems caused by accidentally using the plain builder instead of the one created by the central functions. This should be done in both client classes.

adoroszlai · 2024-06-14T06:31:56Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java

+  public static ContainerCommandRequestProto.Builder getContainerCommandRequestProtoBuilder(
+      ContainerCommandRequestProto req, int version) {
+    return (req == null ?
+        ContainerCommandRequestProto.newBuilder() : ContainerCommandRequestProto.newBuilder(req)).setVersion(version);
+  }


req may already have version set. newBuilder(req) preserves that in the builder, but setVersion overwrites it. This is a problem if called via getContainerCommandRequestProtoBuilder(req), without explicit version.

We would have to build an object again there. Is it something we should do?

If that is the case we should just send a builder than sending the request object. That is also one viable change.

or we can just add a validation check and throw an exception if the version is not set.

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

adoroszlai · 2024-06-14T06:36:38Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java


-      ContainerCommandRequestProto finalPayload =
-          ContainerCommandRequestProto.newBuilder(request)
+      ContainerCommandRequestProto finalPayload = getContainerCommandRequestProtoBuilder(request)


Also set version conditionally here.

Change-Id: I24026f8f2863c1b0501c781452c269a09a7c3b7c

Change-Id: I510d453b900c945814c4d665dfcee5aa14ab0476

Change-Id: I5524a82d0c77a9c01a9034cb4cd0cd2ae577b3f8

Change-Id: Icca0406b6cd1cf9edb8bd0dc350a537a397737a3

Change-Id: Ie101e536af7728528036a042437ff23896c3f34e

hemantk-12

LGTM.

hemantk-12 · 2024-06-15T00:47:19Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

    List<DatanodeDetails> datanodeList = pipeline.getNodes();
    HashMap<DatanodeDetails, CompletableFuture<ContainerCommandResponseProto>>
            futureHashMap = new HashMap<>();
+    ContainerCommandRequestProto.Builder builder = ContainerCommandRequestProto.newBuilder(request);


nit: create a helper function something likegetVersionRequest() and reuse it in other places.

adoroszlai

Thanks a lot @swamirishi for updating the patch. I think it's much simpler now. Only few nits picked.

adoroszlai · 2024-06-16T18:12:38Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

+    if (!request.hasVersion()) {
+      builder.setVersion(ClientVersion.CURRENT.toProtoValue());
+    }
+    ContainerCommandRequestProto finalRequest = builder.build();


Here we only conditionally need to create newBuilder() and then build().

adoroszlai · 2024-06-16T18:13:48Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

    return TracingUtil.executeInNewSpan(spanName,
        () -> {
-          ContainerCommandRequestProto finalPayload =
+          ContainerCommandRequestProto.Builder finalPayload =


nit: finalPayload can be renamed to builder.

adoroszlai · 2024-06-16T18:14:02Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

    try (Scope ignored = GlobalTracer.get().activateSpan(span)) {

-      ContainerCommandRequestProto finalPayload =
+      ContainerCommandRequestProto.Builder finalPayload =


nit: finalPayload can be renamed to builder.

adoroszlai · 2024-06-16T18:14:29Z

...lient/src/test/java/org/apache/hadoop/hdds/scm/storage/TestBlockOutputStreamCorrectness.java

    public XceiverClientReply sendCommandAsync(
        ContainerCommandRequestProto request
    ) {
+      final ContainerCommandRequestProto.Builder finalRequest = ContainerCommandRequestProto.newBuilder(request);


nit: finalRequest can be renamed to builder.

adoroszlai · 2024-06-16T18:14:59Z

...lient/src/test/java/org/apache/hadoop/hdds/scm/storage/TestBlockOutputStreamCorrectness.java

+      final ContainerCommandRequestProto.Builder finalRequest = ContainerCommandRequestProto.newBuilder(request);
+      if (!request.hasVersion()) {
+        finalRequest.setVersion(ClientVersion.CURRENT.toProtoValue());
+      }
+      request = finalRequest.build();


Here we only conditionally need to create newBuilder() and then build().

adoroszlai · 2024-06-16T18:15:44Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/container/ContainerTestHelper.java

-    final Builder builder =
-        ContainerCommandRequestProto.newBuilder()
+      ClientVersion clientVersion, ContainerProtos.Type cmdType, int replicaIndex) {
+    final Builder builder = ContainerCommandRequestProto.newBuilder().setVersion(clientVersion.toProtoValue())


nit:

Suggested change

final Builder builder = ContainerCommandRequestProto.newBuilder().setVersion(clientVersion.toProtoValue())

final Builder builder = ContainerCommandRequestProto.newBuilder()

.setVersion(clientVersion.toProtoValue())

Change-Id: If914929863aba7e7123f3daf73e682e1416967b1

adoroszlai

Thanks @swamirishi for updating the patch, LGTM.

swamirishi · 2024-06-17T20:59:35Z

Thank your for the review @hemantk-12 @adoroszlai

…oto (apache#6812) (cherry picked from commit 81bc179)

…andRequestProto (apache#6812) (cherry picked from commit 81bc179) Change-Id: I1a11a59f7d245578ff233a29c9b120974c5a413d

swamirishi added 2 commits June 13, 2024 11:58

HDDS-11013. Refactor Container service request to set the client vers…

69f92e2

…ion in one centralised function Change-Id: I540e518819db3ebfec1aca5b3a31d1b5ba5fee41

Merge remote-tracking branch 'apache/master' into HEAD

75c5f80

Change-Id: Id5abeebc078d39d15d516cf804dcdaaa3ed2467c Change-Id: Ic5a87d9e696a462cfe92ff35a0068dfeba97541c

swamirishi requested review from adoroszlai and sodonnel June 13, 2024 20:20

swamirishi mentioned this pull request Jun 13, 2024

HDDS-10983. EC Key read corruption when the replica index of container in DN mismatches #6779

Merged

swamirishi requested a review from hemantk-12 June 13, 2024 20:26

hemantk-12 reviewed Jun 13, 2024

View reviewed changes

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/ClientVersion.java Outdated Show resolved Hide resolved

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/ContainerProtocolCalls.java Outdated Show resolved Hide resolved

HDDS-11013. Address review comments

d0c2819

Change-Id: I8936e48570d75052a32c40a9aee751628b753585

swamirishi requested a review from hemantk-12 June 14, 2024 00:56

hemantk-12 approved these changes Jun 14, 2024

View reviewed changes

adoroszlai reviewed Jun 14, 2024

View reviewed changes

adoroszlai changed the title ~~HDDS-11013. Refactor Container service request to set the client version in one centralised function~~ HDDS-11013. Centralise ContainerCommandRequestProto.Builder creation to ensure version is set Jun 14, 2024

adoroszlai requested changes Jun 14, 2024

View reviewed changes

adoroszlai changed the title ~~HDDS-11013. Centralise ContainerCommandRequestProto.Builder creation to ensure version is set~~ HDDS-11013. Ensure version is always set in ContainerCommandRequestProto Jun 14, 2024

swamirishi added 4 commits June 14, 2024 06:24

Merge remote-tracking branch 'apache/master' into HEAD

f61dc64

Change-Id: I24026f8f2863c1b0501c781452c269a09a7c3b7c

HDDS-11013. Address review comments

676780b

Change-Id: I510d453b900c945814c4d665dfcee5aa14ab0476

HDDS-11013. Address review comments

f430283

Change-Id: I5524a82d0c77a9c01a9034cb4cd0cd2ae577b3f8

HDDS-11013. Remove refactoring change

36c1baf

Change-Id: Icca0406b6cd1cf9edb8bd0dc350a537a397737a3

swamirishi requested a review from adoroszlai June 14, 2024 22:24

HDDS-11013. Fix version

f922a59

Change-Id: Ie101e536af7728528036a042437ff23896c3f34e

swamirishi marked this pull request as draft June 14, 2024 22:32

swamirishi requested a review from hemantk-12 June 14, 2024 23:56

swamirishi marked this pull request as ready for review June 14, 2024 23:56

hemantk-12 approved these changes Jun 15, 2024

View reviewed changes

adoroszlai reviewed Jun 16, 2024

View reviewed changes

HDDS-11013. Address review comments

861545c

Change-Id: If914929863aba7e7123f3daf73e682e1416967b1

adoroszlai approved these changes Jun 17, 2024

View reviewed changes

swamirishi merged commit 81bc179 into apache:master Jun 17, 2024

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024

HDDS-11013. Ensure version is always set in ContainerCommandRequestPr…

50a97dc

…oto (apache#6812) (cherry picked from commit 81bc179)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024

HDDS-11013. Ensure version is always set in ContainerCommandRequestPr…

3da511f

…oto (apache#6812) (cherry picked from commit 81bc179)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 18, 2024

HDDS-11013. Ensure version is always set in ContainerCommandRequestPr…

ffdba74

…oto (apache#6812) (cherry picked from commit 81bc179)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 18, 2024

HDDS-11013. Ensure version is always set in ContainerCommandRequestPr…

5fde0fb

…oto (apache#6812) (cherry picked from commit 81bc179)

xichen01 mentioned this pull request Jul 18, 2024

[DO NOT MERGE] Backport some fixes, performance optimizations from master to ozone-1.4 #6929 #6964

Merged

ptlrs pushed a commit to ptlrs/ozone that referenced this pull request Mar 8, 2025

CDPD-71328. HDDS-11013. Ensure version is always set in ContainerComm…

fe0d7e3

…andRequestProto (apache#6812) (cherry picked from commit 81bc179) Change-Id: I1a11a59f7d245578ff233a29c9b120974c5a413d

	final Builder builder = ContainerCommandRequestProto.newBuilder().setVersion(clientVersion.toProtoValue())
	final Builder builder = ContainerCommandRequestProto.newBuilder()
	.setVersion(clientVersion.toProtoValue())

HDDS-11013. Ensure version is always set in ContainerCommandRequestProto #6812

HDDS-11013. Ensure version is always set in ContainerCommandRequestProto #6812

Uh oh!

Conversation

swamirishi commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

swamirishi commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swamirishi commented Jun 14, 2024

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai 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!

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!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishi commented Jun 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

swamirishi commented Jun 13, 2024 •

edited

Loading

swamirishi commented Jun 14, 2024 •

edited

Loading

adoroszlai left a comment •

edited

Loading