HDDS-16365. Pipeline.getReplicaIndexes() unnecessarily create a new map. - #11194
HDDS-16365. Pipeline.getReplicaIndexes() unnecessarily create a new map.#11194szetszwo wants to merge 3 commits into
Conversation
| .setContainerID(containerBlockID.getContainerID()) | ||
| .setLocalID(containerBlockID.getLocalID()) | ||
| .setBlockCommitSequenceId(blockCommitSequenceId); | ||
| if (replicaIdx != null && replicaIdx > 0) { |
There was a problem hiding this comment.
This drops explicit replica index 0 on re-serialization. A base/head probe (setReplicaIndex(0) -> getFromProtobuf -> serialize) keeps presence on base but loses it here. Please preserve explicit zero and add a round-trip test.
| return true; | ||
| } | ||
|
|
||
| public Map<DatanodeDetails, Integer> getReplicaIndexesForTesting() { |
There was a problem hiding this comment.
This renames the ozone admin pipeline list --json field from replicaIndexes to replicaIndexesForTesting, since Pipeline is serialized directly here. A base/head JsonUtils probe reproduces it. Please preserve the existing property name.
| return Collections.unmodifiableList(validators); | ||
| } | ||
|
|
||
| public static HashMap<DatanodeDetails, GetBlockResponseProto> |
There was a problem hiding this comment.
Please avoid removing this public method in HDDS-16365. It is unrelated, was added by HDDS-3867, and exists in released Ozone versions; please restore it or move the cleanup to a follow-up Jira.
There was a problem hiding this comment.
@rich7420 , ContainerProtocolCalls is just a private API although the methods are public. It has nothing to do if it is already in released Ozone versions.
Creating new JIRAs for removing unused methods is wasting our resource. Let's do it here?
There was a problem hiding this comment.
Oh ok, thanks for the explaination
chungen0126
left a comment
chungen0126
left a comment
There was a problem hiding this comment.
+1 LGTM Just some nits.
| return true; | ||
| } | ||
|
|
||
| public Map<DatanodeDetails, Integer> getReplicaIndexes() { |
There was a problem hiding this comment.
If this method is only used for testing, consider adding the @VisibleForTesting annotation.
There was a problem hiding this comment.
As @rich7420 mentioned, it is used for JSON. Otherwise, I would rename it to getReplicaIndexesForTesting.
…peline/Pipeline.java Co-authored-by: Chung En Lee <wfps1210@gmail.com>
3 participants
What changes were proposed in this pull request?
In Pipeline, the getReplicaIndexes() method, which copies the replicaIndexes map, is not really needed:
For #1 above, getReplicaIndexes() is mainly used for building DatanodeBlockID protos. We will also refactor the code:
What is the link to the Apache JIRA
HDDS-16365
How was this patch tested?
By updating existing tests.