adoroszlai commented on code in PR #10454:
URL: https://github.com/apache/ozone/pull/10454#discussion_r3589157737
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java:
##########
@@ -380,37 +367,91 @@ void removeContainerFromPipeline(PipelineID pipelineID,
ContainerID containerID)
*/
Pipeline updatePipelineState(PipelineID pipelineID, PipelineState state)
throws PipelineNotFoundException {
- Objects.requireNonNull(pipelineID, "Pipeline Id cannot be null");
Objects.requireNonNull(state, "Pipeline LifeCycleState cannot be null");
- final Pipeline pipeline = getPipeline(pipelineID);
+ final PipelineInfo info = getPipeline(pipelineID);
+ final Pipeline pipeline = info.getPipeline();
// Return the old pipeline if updating same state
if (pipeline.getPipelineState() == state) {
LOG.debug("CurrentState and NewState are the same, return from " +
"updatePipelineState directly.");
return pipeline;
}
- Pipeline updatedPipeline = pipelineMap.compute(pipelineID,
- (id, p) -> pipeline.toBuilder().setState(state).build());
+ final Pipeline updated = pipeline.toBuilder().setState(state).build();
+ PipelineInfo oldInfo = getPipeline(pipelineID);
Review Comment:
`oldInfo` is the same as `info` from:
https://github.com/apache/ozone/blob/04a965d28c1dcc25de98e631c46dc605c02c2719/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java#L372
I think we only need one lookup.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java:
##########
@@ -471,7 +472,7 @@ protected void removePipeline(Pipeline pipeline)
*/
private void closeContainersForPipeline(final PipelineID pipelineId)
throws IOException {
- Set<ContainerID> containerIDs = stateManager.getContainers(pipelineId);
+ NavigableSet<ContainerID> containerIDs = new
TreeSet<>(stateManager.getContainers(pipelineId));
Review Comment:
Previously this copy (`new TreeSet`) happened:
https://github.com/apache/ozone/blob/398709d6b68312306149144795c5796d4d3fa8b3/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java#L299-L309
while holding read lock:
https://github.com/apache/ozone/blob/398709d6b68312306149144795c5796d4d3fa8b3/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L206-L208
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateMap.java:
##########
@@ -380,37 +367,91 @@ void removeContainerFromPipeline(PipelineID pipelineID,
ContainerID containerID)
*/
Pipeline updatePipelineState(PipelineID pipelineID, PipelineState state)
throws PipelineNotFoundException {
- Objects.requireNonNull(pipelineID, "Pipeline Id cannot be null");
Objects.requireNonNull(state, "Pipeline LifeCycleState cannot be null");
- final Pipeline pipeline = getPipeline(pipelineID);
+ final PipelineInfo info = getPipeline(pipelineID);
+ final Pipeline pipeline = info.getPipeline();
// Return the old pipeline if updating same state
if (pipeline.getPipelineState() == state) {
LOG.debug("CurrentState and NewState are the same, return from " +
"updatePipelineState directly.");
return pipeline;
}
- Pipeline updatedPipeline = pipelineMap.compute(pipelineID,
- (id, p) -> pipeline.toBuilder().setState(state).build());
+ final Pipeline updated = pipeline.toBuilder().setState(state).build();
+ PipelineInfo oldInfo = getPipeline(pipelineID);
+
+ PipelineInfo newInfo = new PipelineInfo(updated);
+
+ for (ContainerID cid : oldInfo.copyContainers()) {
+ newInfo.addContainer(cid);
+ }
Review Comment:
Why do we need to `copyContainers` here? This is called while holding write
lock in `PipelineStateManagerImpl`:
https://github.com/apache/ozone/blob/04a965d28c1dcc25de98e631c46dc605c02c2719/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineStateManagerImpl.java#L277-L283
I think we can use `getContainers()` instead.
--
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]
