HDDS-4731. Fix compatibility issue caused by new enum DatanodeDetails.Port.Name.REPLICATION by adoroszlai · Pull Request #1844 · apache/ozone

adoroszlai · 2021-01-25T21:40:46Z

What changes were proposed in this pull request?

Introduce version number to client messages. Old client does not send version number, in this case it defaults to 0 at the other side. Client with this change sends version = 1.
Only include pre-existing ports of datanode (ie. STANDALONE, RATIS, REST) in DatanodeDetails protobuf message sent to v0 client, which cannot handle new enum constants.
Make client more future-proof by ignoring unknown Port.Name values. This in itself is not enough as a fix, since old client does not have this logic. But this allows the server to send any new port from now on to v >= 1 clients.

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

How was this patch tested?

Added acceptance test which verifies cross-compatibility of key write/read among 0.5.0, 1.0.0, 1.1.0 versions.

Tested closed container replication using ozone-topology compose environment to check for regression.

https://github.com/adoroszlai/hadoop-ozone/actions/runs/510263073

adoroszlai · 2021-01-25T21:43:37Z

@GlenGeng

GlenGeng-awx · 2021-01-26T01:20:45Z

We can help to do a upgrade test to verify whether this issue has been solved in the real environment. Please let me know when I can start the verification.
cc @Xushaohong who helped to find this issue.

… block location

adoroszlai · 2021-01-26T08:44:33Z

We can help to do a upgrade test to verify whether this issue has been solved in the real environment. Please let me know when I can start the verification.

Thanks @GlenGeng for the offer. Is it repeatable (ie. after the test can you restore prior state), or do we have only one shot?

GlenGeng-awx · 2021-01-26T09:04:41Z

It is repeatable. Just setup an ozone cluster with your dev branch, and run a testdfsio with old jars upon it.

adoroszlai · 2021-01-26T10:07:18Z

It is repeatable. Just setup an ozone cluster with your dev branch, and run a testdfsio with old jars upon it.

Thanks, but I meant your test in real environment. I already added a test to reproduce the bug in CI.

GlenGeng-awx · 2021-01-26T11:16:34Z

Thanks @adoroszlai.
The upgrade test was done in a temporary cluster. During the test, an empty ozone cluster with the new version is setup, a yarn and mapreduce with classpath of jars from old ozone is setup, and we just check the compatibility by running a testDFSIO. Everything is in sandbox.

adoroszlai · 2021-01-27T16:04:21Z

On second thought it seems necessary to detect at server-side whether client is old (supports only specific port types) or new (can handle unknown ports). Otherwise no new ports for client can be introduced. The current patch should be fine for the REPLICATION port, since it is for datanodes only.

elek · 2021-01-28T10:39:04Z

On second thought it seems necessary to detect at server-side whether client is old (supports only specific port types) or new (can handle unknown ports). Otherwise no new ports for client can be introduced. The current patch should be fine for the REPLICATION port, since it is for datanodes only.

Yes, I have the same thoughts. Current patch makes the new port internal, which is fine. But even after this patch we can introduce new public ports only after Ozone 2.0 (but I prefer to not break compatibility even between Ozone 1.x client and Ozone 2.x servers.)

adoroszlai · 2021-01-29T14:29:22Z

@GlenGeng @Xushaohong can you please review, too?

GlenGeng-awx

LGTM. Thanks for the fix.

GlenGeng-awx · 2021-02-01T07:11:44Z

hadoop-hdds/interface-client/src/main/proto/DatanodeClientProtocol.proto

  optional   GetCommittedBlockLengthRequestProto getCommittedBlockLength = 22;
+
  optional   string encodedToken = 23;
+  optional uint32 version = 24;


better keep the style consistent. Up to you.

GlenGeng-awx · 2021-02-01T07:13:54Z

hadoop-hdds/interface-server/src/main/proto/ScmServerProtocol.proto

  optional string traceID = 2;

  optional UserInfo userInfo = 3;
+  optional uint32 version = 4;


Will clientVersion be more straightforward than version in these protos, in case we might add other version into proto message ?

They are the same.

xiaoyuyao

Thanks @adoroszlai for working on this. The patch looks good to me, especially I like the new forward/backward compatibility testes added. Just one minor suggestion added inline.

xiaoyuyao · 2021-02-01T18:00:19Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java

+  }
+
+  public HddsProtos.DatanodeDetailsProto.Builder toProtoBuilder(
+      int clientVersion) {


Can we embed the version info inside the PORT Enum without passing clientVersion as parameter?

This way, we can make the the handling of the version specific deserialization easier for future changes.

Thanks @xiaoyuyao for the review. I'm not sure I understand the proposal. Can you please clarify?

We can embed the minimum version required for handling each port in the enum constants (eg. 0 for existing ports and 1 for the new replication port). But don't we still need the client version to be able to decide whether to include it in the proto message or not?

Yes. We still need the client version. But that can be a static helper without many interface changes to introduce extra parameter.

We still need the client version. But that can be a static helper

This conversion must be done on the server-side, and server can have clients of various versions. How would a static helper work?

elek

+1 the concept looks good to me. Let's merge when others have no more concerns.

But I couldn't resist to add some more thoughts about concepts:

Originally we had a pattern where the serialization are added to each of the objects (under different method names).

After a while we introduced the Codec interface to separate the serialization logic from POJOs, but still we call back to the original methods in most of the cases (just because it would be a big refactor to move everything to the codec).

And Codec implementations are used mainly for the persistence.

But long-term I would prefer to use Codec for network serialization, too, to separate the serialization logic and manage in the same way.

At that time, we can introduce VersionedCodec which can use different Codec (depends on the versions).

But this is just a dream, would be great to have more time to improve the code style/class hierarchy....

But even if it's just a dream, I am interested about your opinions ;-)

arp7 · 2021-02-02T17:22:30Z

I merged this based on +1s from @xiaoyuyao and @elek.

adoroszlai · 2021-02-02T17:29:04Z

Thanks @GlenGeng, @elek and @xiaoyuyao for the review, @arp7 for merging it.

arp7 · 2021-02-02T17:33:58Z

It looks like I missed a minor remaining comment from @xiaoyuyao. Let's address in a follow up jira if needed.

adoroszlai added 5 commits January 25, 2021 19:12

Reproduce the bug with cross-compatibility test

206e9af

Avoid duplicate build()

bdcbc0b

Ignore unknown port type

749986a

HDDS-4731. Only include public ports in DatanodeDetails proto by default

95ae22d

Fix missing version number

53b9ce0

adoroszlai self-assigned this Jan 25, 2021

adoroszlai requested review from avijayanhwx and elek January 25, 2021 21:41

adoroszlai requested a review from xiaoyuyao January 25, 2021 21:44

adoroszlai added 5 commits January 26, 2021 04:49

Also log replication source

06a517d

Include ports in DatanodeDetails toString

9d910b8

Add integration test

7672919

Include all ports by default, explicitly limit ports for pipeline and…

bf0921a

… block location

trigger new CI check

221871f

adoroszlai marked this pull request as draft January 27, 2021 16:01

adoroszlai marked this pull request as ready for review January 27, 2021 22:39

adoroszlai added bug Something isn't working upgrade labels Jan 28, 2021

adoroszlai added 2 commits January 28, 2021 19:49

Add client version to proto requests

73d7a5f

trigger new CI check

1577cb8

adoroszlai requested a review from ChenSammi January 29, 2021 14:29

GlenGeng-awx reviewed Feb 1, 2021

View reviewed changes

xiaoyuyao reviewed Feb 1, 2021

View reviewed changes

adoroszlai added 2 commits February 2, 2021 06:51

Merge remote-tracking branch 'origin/master' into HDDS-4731

98b22b7

adjust whitespace

b8bff1a

elek approved these changes Feb 2, 2021

View reviewed changes

arp7 merged commit 5f3e202 into apache:master Feb 2, 2021

guihecheng mentioned this pull request Mar 10, 2021

(Moved) HDDS-4181. Add acceptance tests for upgrade, finalization and downgrade. #1961

Closed

adoroszlai deleted the HDDS-4731 branch March 4, 2022 14:12

Conversation

adoroszlai commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

adoroszlai commented Jan 25, 2021

Uh oh!

GlenGeng-awx commented Jan 26, 2021

Uh oh!

adoroszlai commented Jan 26, 2021

Uh oh!

GlenGeng-awx commented Jan 26, 2021

Uh oh!

adoroszlai commented Jan 26, 2021

Uh oh!

GlenGeng-awx commented Jan 26, 2021

Uh oh!

adoroszlai commented Jan 27, 2021

Uh oh!

elek commented Jan 28, 2021

Uh oh!

adoroszlai commented Jan 29, 2021

Uh oh!

GlenGeng-awx left a comment

Choose a reason for hiding this comment

Uh oh!

GlenGeng-awx Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

GlenGeng-awx Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

adoroszlai Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

xiaoyuyao left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoyuyao Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

adoroszlai Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

xiaoyuyao Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

adoroszlai Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

elek left a comment

Choose a reason for hiding this comment

Uh oh!

arp7 commented Feb 2, 2021

Uh oh!

adoroszlai commented Feb 2, 2021

Uh oh!

arp7 commented Feb 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adoroszlai commented Jan 25, 2021 •

edited

Loading