You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/11/17 18:50:19 UTC
[GitHub] [ozone] aswinshakil opened a new pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
aswinshakil opened a new pull request #2847:
URL: https://github.com/apache/ozone/pull/2847
## What changes were proposed in this pull request?
Change the sequence of steps during pipeline close to,
1. Close containers
2. Close pipelines
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5973
## How was this patch tested?
This patch was tested using Unit test and CI tests.
https://github.com/aswinshakil/ozone/actions/runs/1470167230
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r752668381
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelineManagerImpl.java
##########
@@ -650,6 +650,30 @@ public void testAddContainerWithClosedPipeline() throws Exception {
pipelineID + " in closed state"));
}
+ @Test
+ public void testPipelineCloseFlow() throws IOException {
+ GenericTestUtils.LogCapturer logCapturer = GenericTestUtils.LogCapturer
+ .captureLogs(LoggerFactory.getLogger(PipelineManagerImpl.class));
+ PipelineManagerImpl pipelineManager = createPipelineManager(true);
+ Pipeline pipeline = pipelineManager.createPipeline(
+ new RatisReplicationConfig(HddsProtos.ReplicationFactor.THREE));
+ PipelineID pipelineID = pipeline.getId();
+ pipelineManager.addContainerToPipeline(pipelineID, ContainerID.valueOf(1));
+ pipelineManager.closePipeline(pipeline, false);
+
+ String containerExpectedOutput =
+ "Containers closed for pipeline=" + pipeline;
+ String pipelineExpectedOutput =
+ "Pipeline " + pipeline + " moved to CLOSED state";
+ String logOutput = logCapturer.getOutput();
+ assertTrue(logOutput.contains(containerExpectedOutput));
+ assertTrue(logOutput.contains(pipelineExpectedOutput));
+
+ int containerLogIdx = logOutput.indexOf(containerExpectedOutput);
Review comment:
Sounds good. I will make the changes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r752667531
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java
##########
@@ -341,6 +341,9 @@ protected void closeContainersForPipeline(final PipelineID pipelineId)
public void closePipeline(Pipeline pipeline, boolean onTimeout)
throws IOException {
PipelineID pipelineID = pipeline.getId();
+ // close containers.
+ closeContainersForPipeline(pipelineID);
Review comment:
Thank you for the review and suggestions. I will update the PR with this change.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#issuecomment-973168998
And also once we have this, do we still need HDDS-5843??
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r760684747
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
##########
@@ -1497,7 +1497,7 @@ private void sendCloseCommand(final ContainerInfo container,
}
private String getContainerToken(ContainerID containerID) {
- StorageContainerManager scm = scmContext.getScm();
+ StorageContainerManager scm = (StorageContainerManager) scmContext.getScm();
Review comment:
Do we need to update like above?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bshashikant commented on pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#issuecomment-981288557
@aswinshakil , can you please look at the test failures. The failures are related to recon server and it they look related to this patch
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r760689743
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMContext.java
##########
@@ -186,7 +188,9 @@ public long getTermOfLeader() throws NotLeaderException {
if (!isLeader) {
LOG.warn("getTerm is invoked when not leader.");
- throw scm.getScmHAManager()
+ StorageContainerManager storageContainerManager =
Review comment:
Yes we should do this for only StorageContainerManager objects as this operation is specific to 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#issuecomment-973327198
I don't think we need [HDDS-5843](https://issues.apache.org/jira/browse/HDDS-5843) after this fix, Unless if there is any other way we can be in a situation where the pipeline is open and the container is closed. Your thoughts on that @bharatviswa504?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#issuecomment-987131254
Thank You everyone for the reviews and @aswinshakil for the contribution
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r752530129
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java
##########
@@ -341,6 +341,9 @@ protected void closeContainersForPipeline(final PipelineID pipelineId)
public void closePipeline(Pipeline pipeline, boolean onTimeout)
throws IOException {
PipelineID pipelineID = pipeline.getId();
+ // close containers.
+ closeContainersForPipeline(pipelineID);
Review comment:
Current logic of closeContainersForpipeline is just firing events, so lets say events have not completed and pipeline closed or removed we will be in situation like containers open with out a pipeline.
So, I think we should update closeContainersForPipeline as below.
With this approach we are making sure containers are in closing before pipeline remove/close log in ratis.
```
protected void closeContainersForPipeline(final PipelineID pipelineId)
throws IOException {
Set<ContainerID> containerIDs = stateManager.getContainers(pipelineId);
for (ContainerID containerID : containerIDs) {
if (scmContext.getScm().getContainerManager()
.getContainer(containerID).getState()
== HddsProtos.LifeCycleState.OPEN) {
try {
scmContext.getScm().getContainerManager().updateContainerState(
containerID, HddsProtos.LifeCycleEvent.FINALIZE);
} catch (InvalidNodeStateException ex) {
throw new IOException(ex);
}
}
eventPublisher.fireEvent(SCMEvents.CLOSE_CONTAINER, containerID);
}
}
```
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestPipelineManagerImpl.java
##########
@@ -650,6 +650,30 @@ public void testAddContainerWithClosedPipeline() throws Exception {
pipelineID + " in closed state"));
}
+ @Test
+ public void testPipelineCloseFlow() throws IOException {
+ GenericTestUtils.LogCapturer logCapturer = GenericTestUtils.LogCapturer
+ .captureLogs(LoggerFactory.getLogger(PipelineManagerImpl.class));
+ PipelineManagerImpl pipelineManager = createPipelineManager(true);
+ Pipeline pipeline = pipelineManager.createPipeline(
+ new RatisReplicationConfig(HddsProtos.ReplicationFactor.THREE));
+ PipelineID pipelineID = pipeline.getId();
+ pipelineManager.addContainerToPipeline(pipelineID, ContainerID.valueOf(1));
+ pipelineManager.closePipeline(pipeline, false);
+
+ String containerExpectedOutput =
+ "Containers closed for pipeline=" + pipeline;
+ String pipelineExpectedOutput =
+ "Pipeline " + pipeline + " moved to CLOSED state";
+ String logOutput = logCapturer.getOutput();
+ assertTrue(logOutput.contains(containerExpectedOutput));
+ assertTrue(logOutput.contains(pipelineExpectedOutput));
+
+ int containerLogIdx = logOutput.indexOf(containerExpectedOutput);
Review comment:
We can put a log in closeContainers for pipeline, instead of adding generic log like mentioning
Containers closed for pipeline={}, We can change it as
LOG.info("Container {} closed for pipeline={}, containerID, pipelineID) we can add this to closeContainersForPipeline.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 edited a comment on pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#issuecomment-973168998
And also once we have this fix, do we still need HDDS-5843??
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r760687133
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMContext.java
##########
@@ -186,7 +188,9 @@ public long getTermOfLeader() throws NotLeaderException {
if (!isLeader) {
LOG.warn("getTerm is invoked when not leader.");
- throw scm.getScmHAManager()
+ StorageContainerManager storageContainerManager =
Review comment:
So, should we do this only for when it is StorageContainerManager object lke above
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMContext.java
##########
@@ -186,7 +188,9 @@ public long getTermOfLeader() throws NotLeaderException {
if (!isLeader) {
LOG.warn("getTerm is invoked when not leader.");
- throw scm.getScmHAManager()
+ StorageContainerManager storageContainerManager =
Review comment:
Here if it is called from ReconContext it will fail with ClassCastException right?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r760684747
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
##########
@@ -1497,7 +1497,7 @@ private void sendCloseCommand(final ContainerInfo container,
}
private String getContainerToken(ContainerID containerID) {
- StorageContainerManager scm = scmContext.getScm();
+ StorageContainerManager scm = (StorageContainerManager) scmContext.getScm();
Review comment:
Do we need to update like above, as now scm cannot be null?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] avijayanhwx commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
avijayanhwx commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r752614389
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java
##########
@@ -341,6 +341,9 @@ protected void closeContainersForPipeline(final PipelineID pipelineId)
public void closePipeline(Pipeline pipeline, boolean onTimeout)
throws IOException {
PipelineID pipelineID = pipeline.getId();
+ // close containers.
+ closeContainersForPipeline(pipelineID);
Review comment:
+1 to the above suggestion. We cannot rely on async handling of moving container from OPEN to CLOSING.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bshashikant commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r753940074
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java
##########
@@ -341,6 +341,9 @@ protected void closeContainersForPipeline(final PipelineID pipelineId)
public void closePipeline(Pipeline pipeline, boolean onTimeout)
throws IOException {
PipelineID pipelineID = pipeline.getId();
+ // close containers.
+ closeContainersForPipeline(pipelineID);
Review comment:
+1 on the idea
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bshashikant commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r753940074
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java
##########
@@ -341,6 +341,9 @@ protected void closeContainersForPipeline(final PipelineID pipelineId)
public void closePipeline(Pipeline pipeline, boolean onTimeout)
throws IOException {
PipelineID pipelineID = pipeline.getId();
+ // close containers.
+ closeContainersForPipeline(pipelineID);
Review comment:
+1 n the idea
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2847:
URL: https://github.com/apache/ozone/pull/2847#discussion_r755083448
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineManagerImpl.java
##########
@@ -341,6 +341,9 @@ protected void closeContainersForPipeline(final PipelineID pipelineId)
public void closePipeline(Pipeline pipeline, boolean onTimeout)
throws IOException {
PipelineID pipelineID = pipeline.getId();
+ // close containers.
+ closeContainersForPipeline(pipelineID);
Review comment:
+1, the idea looks good！
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 merged pull request #2847: HDDS-5973. Changed sequence of steps during pipeline close.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #2847:
URL: https://github.com/apache/ozone/pull/2847
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org