HDDS-10422. Fix some warnings about exposing internal representation in hdds-common #6351

adoroszlai · 2024-03-08T09:19:35Z

What changes were proposed in this pull request?

Fix some Spotbugs (4.x) warnings that code may expose internal representation. Notable changes:

~~OzoneQuota used to initalize contents of QuotaList, moved to the latter~~ (deferred to HDDS-10896)
~~move creation of mutable metrics objects to PerformanceMetrics~~ (deferred to HDDS-10897)

Also remove unused hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/BatchOperation.java (seems to be outdated duplicate of hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/BatchOperation.java).

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

How was this patch tested?

CI:
https://github.com/adoroszlai/ozone/actions/runs/8200024023

…common

szetszwo

@adoroszlai , Thanks for working on this! Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13068960/6351_review.patch

BTW, this change is quite big. How about separating it into at least two JIRAs?

szetszwo · 2024-05-18T17:09:45Z

...op-hdds/common/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ChunkInfo.java

-    if (info.hasStripeChecksum()) {
-      chunkInfo.setStripeChecksum(info.getStripeChecksum());
-    }


Is it the case that info.hasStripeChecksum() is always false? If yes, how about adding a precondition?

No, ChunkInfo proto may have stripe checksum. However, getStripeChecksum of the non-proto ChunkInfo is never called. I'll remove this part of the change.

szetszwo · 2024-05-18T18:20:28Z

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

@@ -222,7 +222,7 @@ public synchronized void setPort(Name name, int port) {
   * @return DataNode Ports
   */
  public synchronized List<Port> getPorts() {
-    return ports;
+    return new ArrayList<>(ports);


We should also change the callers in order to reduce copying:

NodeDecommissionManager

//NodeDecommissionManager private boolean validateDNPortMatch(int port, DatanodeDetails dn) { return dn.containPort(port); }

//DatanodeDetails public synchronized boolean containPort(int port) { for (Port p : ports) { if (p.getValue() == port) { return true; } } return false; }

DatanodeIdYaml

@@ -238,8 +239,9 @@ private static DatanodeDetailsYaml getDatanodeDetailsYaml( = new DatanodeLayoutStorage(conf, datanodeDetails.getUuidString()); Map<String, Integer> portDetails = new LinkedHashMap<>(); - if (!CollectionUtils.isEmpty(datanodeDetails.getPorts())) { - for (DatanodeDetails.Port port : datanodeDetails.getPorts()) { + final List<DatanodeDetails.Port> datanodePorts = datanodeDetails.getPorts(); + if (!CollectionUtils.isEmpty(datanodePorts)) { + for (DatanodeDetails.Port port : datanodePorts) { Field f = null; try { f = DatanodeDetails.Port.Name.class

Thanks for the suggestion, updated.

szetszwo · 2024-05-18T18:29:23Z

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

@@ -104,6 +105,6 @@ public String getScmId() {
   * @return List of peer address
   */
  public List<String> getRatisPeerRoles() {
-    return peerRoles;
+    return Collections.unmodifiableList(peerRoles);


Do it in constructor.

private ScmInfo(String clusterId, String scmId, List<String> peerRoles) { this.clusterId = clusterId; this.scmId = scmId; this.peerRoles = Collections.unmodifiableList(peerRoles); }

szetszwo · 2024-05-18T18:37:23Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java

@@ -555,13 +555,14 @@ public Builder(Pipeline pipeline) {
      this.creationTimestamp = pipeline.getCreationTimestamp();
      this.suggestedLeaderId = pipeline.getSuggestedLeaderId();
      if (nodeStatus != null) {
-        replicaIndexes = new HashMap<>();
+        Map<DatanodeDetails, Integer> indexes = new HashMap<>();


Use builder:

final ImmutableMap.Builder<DatanodeDetails, Integer> b = ImmutableMap.builder(); for (DatanodeDetails dn : nodeStatus.keySet()) { int index = pipeline.getReplicaIndex(dn); if (index > 0) { b.put(dn, index); } } replicaIndexes = b.build();

szetszwo · 2024-05-18T18:42:13Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java

@@ -598,7 +599,7 @@ public Builder setNodes(List<DatanodeDetails> nodes) {

    public Builder setNodeOrder(List<Integer> orders) {
      // for build from ProtoBuf
-      this.nodeOrder = orders;
+      this.nodeOrder = orders == null ? ImmutableList.of() : ImmutableList.copyOf(orders);


Since the orders is from a proto, let's use this.nodeOrder = Collections.unmodifiableList(orders); to avoid copying.

szetszwo · 2024-05-18T18:45:01Z

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

@@ -47,7 +49,7 @@ public ChecksumData(ChecksumType checksumType, int bytesPerChecksum,
                      List<ByteString> checksums) {
    this.type = checksumType;
    this.bytesPerChecksum = bytesPerChecksum;
-    this.checksums = checksums;
+    this.checksums = checksums == null ? ImmutableList.of() : ImmutableList.copyOf(checksums);


Copy is not needed here since it is from proto or a new list.

this.checksums = Collections.unmodifiableList(checksums);

szetszwo · 2024-05-18T18:56:36Z

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

@@ -45,7 +46,7 @@ public class LeaseCallbackExecutor<T> implements Runnable {
   */
  public LeaseCallbackExecutor(T resource, List<Callable<Void>> callbacks) {
    this.resource = resource;
-    this.callbacks = callbacks;
+    this.callbacks = callbacks == null ? ImmutableList.of() : ImmutableList.copyOf(callbacks);


callbacks always has zero or one element. Let's remove the List<>?

szetszwo · 2024-05-18T18:58:23Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java

-    quotaList.addQuotaList(OZONE_QUOTA_KB, Units.KB, KB);
-    quotaList.addQuotaList(OZONE_QUOTA_B, Units.B, 1L);
-  }
+  private static final QuotaList QUOTA_LIST = new QuotaList();


OzoneQuota used to initalize contents of QuotaList, moved to the latter

The QuotaList class is inefficient. Let's remove it.

Deferred to HDDS-10897.

adoroszlai · 2024-05-22T09:05:20Z

Thanks @szetszwo for the review. Deferred some changes that can be logically grouped to separate tasks: HDDS-10896, HDDS-10897 and HDDS-10899.

szetszwo

+1 the change looks good.

adoroszlai · 2024-05-23T05:22:39Z

Thanks @szetszwo for reviewing and merging this.

…in hdds-common (apache#6351)

…concile-cli * HDDS-10239-container-reconciliation: (296 commits) HDDS-10897. Refactor OzoneQuota (apache#6714) HDDS-10422. Fix some warnings about exposing internal representation in hdds-common (apache#6351) HDDS-10899. Refactor Lease callbacks (apache#6715) HDDS-10890. Increase default value for hdds.container.ratis.log.appender.queue.num-elements (apache#6711) HDDS-10832. Client should switch to streaming based on OpenKeySession replication (apache#6683) HDDS-10435. Support S3 object tags for existing requests (apache#6607) HDDS-10883. Improve logging in Recon for finalising DN logic. (apache#6704) HDDS-8752. Enable TestOzoneRpcClientAbstract#testOverWriteKeyWithAndWithOutVersioning (apache#6702) HDDS-10875. XceiverRatisServer#getRaftPeersInPipeline should be called before XceiverRatisServer#removeGroup (apache#6696) HDDS-10514. Recon - Provide DN decommissioning detailed status and info inline with current CLI command output. (apache#6376) HDDS-10878. Bump zstd-jni to 1.5.6-3 (apache#6701) HDDS-10877. Bump Dropwizard metrics to 3.2.6 (apache#6699) HDDS-10876. Bump jackson to 2.16.2 (apache#6697) HDDS-6116. Remove flaky tag from TestSCMInstallSnapshot (apache#6695) HDDS-2643. TestOzoneDelegationTokenSecretManager#testRenewTokenFailureRenewalTime fails intermittently. HDDS-10699. Refactor ContainerBalancerTask and TestContainerBalancerTask (apache#6537) HDDS-10861. Ozone cli supports default ozone.om.service.id (apache#6680) HDDS-10859. Improve error messages when decommission and maintenance fail-early (apache#6678) HDDS-9031. Upgrade acceptance tests to Docker Compose v2 (apache#6667) HDDS-10559. Add a warning or a check to run repair tool as System user (apache#6574) ... Conflicts: hadoop-ozone/dist/src/main/smoketest/admincli/container.robot

HDDS-10422. Fix some expose internal representation warnings in hdds-…

a4dff06

…common

adoroszlai added the code-cleanup Changes that aim to make code better, without changing functionality. label Mar 8, 2024

adoroszlai self-assigned this Mar 8, 2024

adoroszlai requested review from aswinshakil and xichen01 March 14, 2024 10:26

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

243aac2

adoroszlai changed the title ~~HDDS-10422. Fix some expose internal representation warnings in hdds-common~~ HDDS-10422. Fix some warnings about exposing internal representation in hdds-common Mar 24, 2024

adoroszlai requested a review from duongkame May 6, 2024 17:52

adoroszlai requested a review from szetszwo May 18, 2024 13:07

szetszwo reviewed May 18, 2024

View reviewed changes

adoroszlai added 12 commits May 21, 2024 18:34

keep stripeChecksum

95b86b9

wrap in unmodifiableList in constructor

ee33aa7

use ImmutableMap.Builder

52e26a3

use unmodifiableList

41696d3

fixup for 52e26a3

71f5786

fixup for 41696d3

88b5454

avoid duplicate call

5fd72b9

reduce list copy

31b1578

revert OzoneQuota changes

5ce60b2

revert PerformanceMetrics changes

986ca90

revert change in LeaseCallbackExecutor

11f0c44

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

7a7f980

szetszwo and others added 2 commits May 22, 2024 12:23

apply review suggestion for FakeClusterTopology

70f8a47

fix compile errors in FakeClusterTopology

b9ed7e8

This was referenced May 22, 2024

HDDS-10896. Refactor PerformanceMetrics creation #6712

Merged

HDDS-10897. Refactor OzoneQuota #6714

Merged

HDDS-10899. Refactor Lease callbacks #6715

Merged

adoroszlai requested a review from szetszwo May 22, 2024 18:06

szetszwo approved these changes May 22, 2024

View reviewed changes

szetszwo merged commit 0807a77 into apache:master May 22, 2024
39 checks passed

adoroszlai deleted the HDDS-10422 branch May 23, 2024 05:22

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request May 23, 2024

HDDS-10422. Fix some warnings about exposing internal representation …

f89601e

…in hdds-common (apache#6351)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-10422. Fix some warnings about exposing internal representation in hdds-common #6351

HDDS-10422. Fix some warnings about exposing internal representation in hdds-common #6351

adoroszlai commented Mar 8, 2024 •

edited

Loading

szetszwo left a comment

szetszwo May 18, 2024

adoroszlai May 21, 2024

szetszwo May 18, 2024

adoroszlai May 22, 2024

szetszwo May 18, 2024

szetszwo May 18, 2024

szetszwo May 18, 2024

szetszwo May 18, 2024

szetszwo May 18, 2024

szetszwo May 18, 2024 •

edited

Loading

adoroszlai May 22, 2024

adoroszlai commented May 22, 2024

szetszwo left a comment

adoroszlai commented May 23, 2024

HDDS-10422. Fix some warnings about exposing internal representation in hdds-common #6351

HDDS-10422. Fix some warnings about exposing internal representation in hdds-common #6351

Conversation

adoroszlai commented Mar 8, 2024 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

szetszwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo May 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adoroszlai commented May 22, 2024

szetszwo left a comment

Choose a reason for hiding this comment

adoroszlai commented May 23, 2024

adoroszlai commented Mar 8, 2024 •

edited

Loading

szetszwo May 18, 2024 •

edited

Loading