devmadhuu commented on code in PR #6742:
URL: https://github.com/apache/ozone/pull/6742#discussion_r1635803278
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconContainerManager.java:
##########
@@ -229,11 +229,9 @@ public void addNewContainer(ContainerWithPipeline
containerWithPipeline)
try {
if (containerInfo.getState().equals(HddsProtos.LifeCycleState.OPEN)) {
PipelineID pipelineID = containerWithPipeline.getPipeline().getId();
- // Check if the pipeline is present in Recon
- if (!pipelineManager.containsPipeline(pipelineID)) {
- // Pipeline is not present, add it first.
- LOG.info("Adding new pipeline {} from SCM.", pipelineID);
-
reconPipelineManager.addPipeline(containerWithPipeline.getPipeline());
+ // Check if the pipeline is present in Recon if not add it.
+ if
(reconPipelineManager.addPipeline(containerWithPipeline.getPipeline())) {
+ LOG.info("Added new pipeline {} to Recon pipeline metadata.",
pipelineID);
Review Comment:
```suggestion
LOG.info("Added new pipeline {} to Recon pipeline metadata from
SCM.", pipelineID);
```
Since this pipeline is verified pipeline from scm at caller side, so it is
better, we should have that info as well as it was present in earlier.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineManager.java:
##########
@@ -104,12 +104,13 @@ void initializePipelines(List<Pipeline> pipelinesFromScm)
List<Pipeline> pipelinesInHouse = getPipelines();
LOG.info("Recon has {} pipelines in house.", pipelinesInHouse.size());
for (Pipeline pipeline : pipelinesFromScm) {
- if (!containsPipeline(pipeline.getId())) {
- // New pipeline got from SCM. Recon does not know anything about it,
- // so let's add it.
- LOG.info("Adding new pipeline {} from SCM.", pipeline.getId());
- addPipeline(pipeline);
+ // New pipeline got from SCM. Recon does not know anything about it,
+ // so let's add it.
+ boolean added = addPipeline(pipeline);
+ if (added) {
Review Comment:
```suggestion
if (addPipeline(pipeline)) {
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineManager.java:
##########
@@ -104,12 +104,13 @@ void initializePipelines(List<Pipeline> pipelinesFromScm)
List<Pipeline> pipelinesInHouse = getPipelines();
LOG.info("Recon has {} pipelines in house.", pipelinesInHouse.size());
for (Pipeline pipeline : pipelinesFromScm) {
- if (!containsPipeline(pipeline.getId())) {
- // New pipeline got from SCM. Recon does not know anything about it,
- // so let's add it.
- LOG.info("Adding new pipeline {} from SCM.", pipeline.getId());
- addPipeline(pipeline);
+ // New pipeline got from SCM. Recon does not know anything about it,
Review Comment:
This above comments seems not fit until we find out if Recon contains that
pipeline or not.
```suggestion
// New pipeline got from SCM. Validate If it doesn't exists at
Recon, try adding it.
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java:
##########
@@ -83,9 +83,9 @@ protected void processPipelineReport(PipelineReport report,
throw ex;
}
- LOG.info("Adding new pipeline {} to Recon pipeline metadata.",
- pipelineFromScm);
- reconPipelineManager.addPipeline(pipelineFromScm);
+ if (reconPipelineManager.addPipeline(pipelineFromScm)) {
+ LOG.info("Pipeline {} added to Recon pipeline metadata.",
pipelineFromScm);
Review Comment:
```suggestion
LOG.info("Pipeline {} verified from SCM and added to Recon pipeline
metadata.", pipelineFromScm);
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineManager.java:
##########
@@ -104,12 +104,13 @@ void initializePipelines(List<Pipeline> pipelinesFromScm)
List<Pipeline> pipelinesInHouse = getPipelines();
LOG.info("Recon has {} pipelines in house.", pipelinesInHouse.size());
for (Pipeline pipeline : pipelinesFromScm) {
- if (!containsPipeline(pipeline.getId())) {
- // New pipeline got from SCM. Recon does not know anything about it,
- // so let's add it.
- LOG.info("Adding new pipeline {} from SCM.", pipeline.getId());
- addPipeline(pipeline);
+ // New pipeline got from SCM. Recon does not know anything about it,
+ // so let's add it.
+ boolean added = addPipeline(pipeline);
+ if (added) {
+ LOG.info("Added new pipeline {} from SCM.", pipeline.getId());
} else {
+ LOG.warn("Pipeline {} already exists in Recon pipeline metadata.",
pipeline.getId());
// Recon already has this pipeline. Just update state and creation
// time.
getStateManager().updatePipelineState(
Review Comment:
`removeInvalidPipelines(pipelinesFromScm)` this call seems incorrect to be
in for loop, though it is old code, because we are removing full list from SCM
in every iteration. Also pls check this method is acquiring lock even when
caller of this already acquired lock and this method is just called from a
single caller.
--
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]
