GlenGeng commented on a change in pull request #1371:
URL: https://github.com/apache/hadoop-ozone/pull/1371#discussion_r496496531
##########
File path:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/protocol/commands/CreatePipelineCommand.java
##########
@@ -48,16 +52,37 @@ public CreatePipelineCommand(final PipelineID pipelineID,
this.factor = factor;
this.type = type;
this.nodelist = datanodeList;
+ if (datanodeList.size() ==
+ XceiverServerRatis.DEFAULT_PRIORITY_LIST.size()) {
Review comment:
just forward to the Ctor with `priorityList `.
```
this(pipelineID, factor, type, datanodeList,
new ArrayList<>(Collections.nCopies(datanodeList.size(), 0))
```
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/RatisPipelineProvider.java
##########
@@ -98,8 +115,28 @@ private boolean exceedPipelineNumberLimit(ReplicationFactor
factor) {
return false;
}
+ @VisibleForTesting
+ public LeaderChoosePolicy getLeaderChoosePolicy() {
+ return leaderChoosePolicy;
+ }
+ private List<Integer> getPriorityList(
+ List<DatanodeDetails> dns, DatanodeDetails suggestedLeader) {
+ List<Integer> priorityList = new ArrayList<>();
+
+ for (DatanodeDetails dn : dns) {
+ if (dn.getUuid().equals(suggestedLeader.getUuid())) {
Review comment:
why not use `DatanodeDetails.equals()` ?
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManager.java
##########
@@ -59,7 +59,8 @@ void addContainerToPipeline(PipelineID pipelineId,
ContainerID containerID)
pipelineStateMap.addContainerToPipeline(pipelineId, containerID);
}
- Pipeline getPipeline(PipelineID pipelineID) throws PipelineNotFoundException
{
+ public Pipeline getPipeline(PipelineID pipelineID)
Review comment:
revert unnecessary change.
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/SCMPipelineManager.java
##########
@@ -619,9 +619,9 @@ private void finalizePipeline(PipelineID pipelineId) throws
IOException {
* @throws IOException
*/
protected void destroyPipeline(Pipeline pipeline) throws IOException {
- pipelineFactory.close(pipeline.getType(), pipeline);
// remove the pipeline from the pipeline manager
removePipeline(pipeline.getId());
+ pipelineFactory.close(pipeline.getType(), pipeline);
Review comment:
why need this change ? The sequence of sending SCMCommand and removing
state may affect SCM HA.
##########
File path:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/protocol/commands/CreatePipelineCommand.java
##########
@@ -39,6 +42,7 @@
private final ReplicationFactor factor;
private final ReplicationType type;
private final List<DatanodeDetails> nodelist;
+ private final List<Integer> priorityList;
Review comment:
I consider that, should move `RatisPipelineProvider.getPriorityList()`,
`HIGH_PRIORITY`, `LOW_PRIORITY` here, and replace priorityList in Ctor param as
suggestedLeader.
We can minimize the existence of `priorityList`.
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/RatisPipelineProvider.java
##########
@@ -121,18 +158,24 @@ public Pipeline create(ReplicationFactor factor) throws
IOException {
throw new IllegalStateException("Unknown factor: " + factor.name());
}
+ DatanodeDetails suggestedLeader = leaderChoosePolicy.chooseLeader(
Review comment:
make node manager and pipeline manager be member of leaderChoosePolicy,
so that leaderChoosePolicy can has it own state, which will make future
extension easier.
##########
File path:
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java
##########
@@ -123,6 +126,15 @@ public Instant getCreationTimestamp() {
return creationTimestamp;
}
+ /**
+ * Return the suggested leaderId with high priority of pipeline.
Review comment:
Return the suggested leaderId which has a high priority among DNs of the
pipeline.
##########
File path:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java
##########
@@ -115,6 +116,9 @@
private static final Logger LOG = LoggerFactory
.getLogger(XceiverServerRatis.class);
private static final AtomicLong CALL_ID_COUNTER = new AtomicLong();
+ public static final List<Integer> DEFAULT_PRIORITY_LIST =
Review comment:
why we need this `DEFAULT_PRIORITY_LIST` ?
At all the locations it appears, it dn number does not equal to it, you
always create a new list.
Suggest to remove this static var.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
