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.

Sign up for GitHub

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

Jump to bottom

HDDS-9884. Pass DatanodeVersion to the client #6155

Merged
merged 10 commits into from
Feb 15, 2024
Merged

HDDS-9884. Pass DatanodeVersion to the client #6155

merged 10 commits into from
Feb 15, 2024

Conversation

smengcl
Copy link
Contributor

@smengcl smengcl commented Feb 2, 2024 •
edited
Loading

What changes were proposed in this pull request?

DatanodeDetails has initialVersion and currentVersion (DatanodeVersion, not to be confused with HDDSLayoutFeature) added since HDDS-4730 but they were never passed to the client. The core of this PR is just to add both fields to the proto message and make sure they are correctly serded and passed to the client.

To be used by HDDS-9752 (#5663) or its follow-up patch in order to make sure new clients would still work with old datanodes over the wire.

  • Added currentVersion proto field to be passed from DN to a client.

Note: initialVersion field has been intentionally skipped since it is of no use to the client.

What is the link to the Apache JIRA

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

How was this patch tested?

  • Integration test added to verify that datanode versions are correctly passed to the client. Modifications have been done to HddsDatanodeService and MiniOzoneCluster in order to facilitate that.

@smengcl smengcl added the hbase HBase on Ozone support label Feb 2, 2024
@smengcl smengcl self-assigned this Feb 2, 2024
jojochuang
jojochuang approved these changes Feb 3, 2024
View reviewed changes
Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

LGTM Thanks @smengcl

adoroszlai
adoroszlai previously requested changes Feb 3, 2024
View reviewed changes
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @smengcl for working on this.

The proto part looks good, but I think we should improve the tests (and the part added in production code to enable testing).

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java Outdated Show resolved Hide resolved
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java Outdated Show resolved Hide resolved
@adoroszlai adoroszlai dismissed their stale review February 7, 2024 13:58

patch updated

@jojochuang
Copy link
Contributor

This PR will need to rebase, sorry.

@smengcl
Remove initialVersion from proto.
f43fc7a
@smengcl
Merge remote-tracking branch 'asf/HDDS-7593' into HDDS-9884
3b024e0 
Conflicts:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneCluster.java
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestBlockDataStreamOutput.java
@smengcl
Mock static only when non -1 is set.
7d16cfe
errose28
errose28 reviewed Feb 12, 2024
View reviewed changes
Copy link
Contributor

@errose28 errose28 left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement @smengcl

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneCluster.java Show resolved Hide resolved
...gration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestBlockDataStreamOutput.java Outdated Show resolved Hide resolved
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java Show resolved Hide resolved
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsDatanodeService.java Outdated Show resolved Hide resolved
@kerneltime
Copy link
Contributor

Can this information be deduced from the Service Info that the client establishes? Unless we want mixed version Datanodes (mixed set of Datanodes finalized post upgrade), I don't see why we need to add more load to the protocol executed for every read.

@smengcl
Copy link
Contributor Author

smengcl commented Feb 13, 2024 •
edited
Loading

Can this information be deduced from the Service Info that the client establishes? Unless we want mixed version Datanodes (mixed set of Datanodes finalized post upgrade), I don't see why we need to add more load to the protocol executed for every read.

@kerneltime Do you mean ServiceInfo? It doesn't seem to convey DN version but rather OzoneManagerVersion only.

Also on the DN side, DatanodeVersion differs from HDDSLayoutFeature in that DatanodeVersion is the (RPC) version over wire, while HDDSLayoutFeature is the DN layout version on disk. I currently do not see another clean way of telling the client that the DN is using a newer wire version.

jojochuang
jojochuang reviewed Feb 15, 2024
View reviewed changes
Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

Ok this is good

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneClusterImpl.java
@@ -132,12 +135,14 @@ public class MiniOzoneClusterImpl implements MiniOzoneCluster {
private CertificateClient caClient;
private final Set<AutoCloseable> clients = ConcurrentHashMap.newKeySet();
private SecretKeyClient secretKeyClient;
private static MockedStatic mockDNStatic = Mockito.mockStatic(HddsDatanodeService.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

this doc https://www.baeldung.com/mockito-mock-static-methods
recommends to enclose MockedStatic in a try block to avoid side effects that impacts other tests.

I think it's fine for this case though because we usually just have one mini cluster.

@jojochuang jojochuang merged commit 463a09b into apache:HDDS-7593 Feb 15, 2024
35 checks passed
@jojochuang
Copy link
Contributor

Merged. Thanks @smengcl for the PR and @kerneltime @errose28 @adoroszlai for the reviews.

smengcl added a commit to smengcl/hadoop-ozone that referenced this pull request Mar 6, 2024
@smengcl
HDDS-9884. Pass DatanodeVersion to the client (apache#6155)
f6698d5
chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024
@smengcl @chungen0126
HDDS-9884. Pass DatanodeVersion to the client (apache#6155)
b485938
chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024
@smengcl @chungen0126
HDDS-9884. Pass DatanodeVersion to the client (apache#6155)
698b19d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jojochuang jojochuang jojochuang approved these changes

@adoroszlai adoroszlai adoroszlai left review comments

@errose28 errose28 errose28 left review comments

Assignees

@smengcl smengcl

Labels
hbase HBase on Ozone support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smengcl @jojochuang @kerneltime @adoroszlai @errose28