szetszwo commented on code in PR #6351:
URL: https://github.com/apache/ozone/pull/6351#discussion_r1605819906
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ChunkInfo.java:
##########
@@ -97,10 +93,6 @@ public static ChunkInfo
getFromProtoBuf(ContainerProtos.ChunkInfo info)
chunkInfo.setChecksumData(
ChecksumData.getFromProtoBuf(info.getChecksumData()));
- if (info.hasStripeChecksum()) {
- chunkInfo.setStripeChecksum(info.getStripeChecksum());
- }
Review Comment:
Is it the case that `info.hasStripeChecksum()` is always false? If yes, how
about adding a precondition?
##########
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);
Review Comment:
Do it in constructor.
```java
private ScmInfo(String clusterId, String scmId, List<String> peerRoles) {
this.clusterId = clusterId;
this.scmId = scmId;
this.peerRoles = Collections.unmodifiableList(peerRoles);
}
```
##########
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);
Review Comment:
Copy is not needed here since it is from proto or a new list.
```java
this.checksums = Collections.unmodifiableList(checksums);
```
##########
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);
Review Comment:
Since the `orders` is from a proto, let's use `this.nodeOrder =
Collections.unmodifiableList(orders);` to avoid copying.
##########
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);
Review Comment:
We should also change the callers in order to reduce copying:
- NodeDecommissionManager
```java
//NodeDecommissionManager
private boolean validateDNPortMatch(int port, DatanodeDetails dn) {
return dn.containPort(port);
}
```
```java
//DatanodeDetails
public synchronized boolean containPort(int port) {
for (Port p : ports) {
if (p.getValue() == port) {
return true;
}
}
return false;
}
```
- DatanodeIdYaml
```java
@@ -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
```
##########
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<>();
Review Comment:
Use builder:
```java
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();
```
##########
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);
Review Comment:
`callbacks` always has zero or one element. Let's remove the `List<>`?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/client/OzoneQuota.java:
##########
@@ -44,27 +39,17 @@ public enum Units { B, KB, MB, GB, TB }
// Quota to decide how many buckets can be created.
private long quotaInNamespace;
// Quota to decide how many storage space will be used in bytes.
- private long quotaInBytes;
- private RawQuotaInBytes rawQuotaInBytes;
+ private final long quotaInBytes;
+ private final RawQuotaInBytes rawQuotaInBytes;
// Data class of Quota.
- private static QuotaList quotaList;
-
- /** Setting QuotaList parameters from large to small. */
- static {
- quotaList = new QuotaList();
- quotaList.addQuotaList(OZONE_QUOTA_TB, Units.TB, TB);
- quotaList.addQuotaList(OZONE_QUOTA_GB, Units.GB, GB);
- quotaList.addQuotaList(OZONE_QUOTA_MB, Units.MB, MB);
- quotaList.addQuotaList(OZONE_QUOTA_KB, Units.KB, KB);
- quotaList.addQuotaList(OZONE_QUOTA_B, Units.B, 1L);
- }
+ private static final QuotaList QUOTA_LIST = new QuotaList();
Review Comment:
The `QuotaList` class is inefficient. Let's remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
