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/06/25 08:32:55 UTC
[GitHub] [ozone] JacksonYao287 opened a new pull request #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
JacksonYao287 opened a new pull request #2371:
URL: https://github.com/apache/ozone/pull/2371
## What changes were proposed in this pull request?
if recon receives a pipelineReport , the reconPipelineReportHandler will confirm it with scm. if scm does not find this pipeline , it will throw a pipelineNotFoundExecption. today, if this happen, reconPipelineReportHandler will try again and again. this behavior is not expected. the right behavior is that if this happen, reconPipelineReportHandler should just ignore this report and go ahead.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5390
## How was this patch tested?
no need no test
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r681573763
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,14 +64,28 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (IOException ex) {
+ if (ex instanceof RemoteException) {
+ IOException ioe = ((RemoteException) ex)
+ .unwrapRemoteException(PipelineNotFoundException.class);
+ if (ioe instanceof PipelineNotFoundException) {
Review comment:
And also avoid loggig??
catch(NotLeaderException ex) {
// Avoid NotLeaderException logging which happens when processing
// pipeline report on followers.
} catch(PipelineNotFoundException e) {
// When pipeline not found donot log??
}catch(IOException e) {
LOGGER.error("Could not process pipeline report={} from dn={}.",
report, dn, e);
}
--
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r672971961
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,8 +63,16 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm = null;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (PipelineNotFoundException pnfe) {
Review comment:
````
try {
cluster.getStorageContainerLocationClient().getPipeline(HddsProtos.PipelineID.newBuilder().setId(UUID.randomUUID().toString()).build());
} catch (IOException ex) {
Assert.assertTrue(ex instanceof RemoteException);
PipelineNotFoundException exception = (PipelineNotFoundException) ((RemoteException) ex).unwrapRemoteException(PipelineNotFoundException.class);
Assert.assertTrue(exception instanceof PipelineNotFoundException);
}
````
We donot get PipelineNotFoundException, we get RemoteException wrapped with PipelineNotFoundException. We need to unwrap and check if it is PipelineNotFoundException.
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,8 +63,16 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm = null;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (PipelineNotFoundException pnfe) {
Review comment:
````
try {
cluster.getStorageContainerLocationClient().getPipeline(HddsProtos.PipelineID.newBuilder().setId(UUID.randomUUID().toString()).build());
} catch (IOException ex) {
Assert.assertTrue(ex instanceof RemoteException);
PipelineNotFoundException exception = (PipelineNotFoundException) ((RemoteException) ex).unwrapRemoteException(PipelineNotFoundException.class);
Assert.assertTrue(exception instanceof PipelineNotFoundException);
}
````
You can use this to check by updating test TestStorageContainerManagerHA#testAllSCMAreRunning. (During this found issue in getStorageContainerLocationClient() comment below code.
/* InetSocketAddress address = scm.getClientRpcAddress();*/
/* LOG.info(
"Creating StorageContainerLocationProtocol RPC client with address {}",
address);*/
We donot get PipelineNotFoundException, we get RemoteException wrapped with PipelineNotFoundException. We need to unwrap and check if it is PipelineNotFoundException.
--
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#issuecomment-895167222
Thank You @JacksonYao287 for the fix.
--
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658651701
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java
##########
@@ -70,6 +71,9 @@ private HddsClientUtils() {
// Not Replicated Exception will be thrown if watch For commit
// does not succeed
.add(NotReplicatedException.class)
+ // for reconPipelineReportHandler , if could not find pipeline
Review comment:
And also we don't use this, we use the list specified in
```
private static final List<Class<? extends Exception>>
NON_RETRIABLE_EXCEPTION_LIST =
ImmutableList.<Class<? extends Exception>>builder()
.add(SCMException.class)
.add(NonRetriableException.class)
.build();
```
SCMHAUtils.java
And also you can refer SCMHAUtils#getRetryAction for this.
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658714135
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java
##########
@@ -70,6 +71,9 @@ private HddsClientUtils() {
// Not Replicated Exception will be thrown if watch For commit
// does not succeed
.add(NotReplicatedException.class)
+ // for reconPipelineReportHandler , if could not find pipeline
Review comment:
do you mean if i want to forbid this retry here for pipelineNotFoundException , i should set it in `SCMHAUtils.java`?
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658654635
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,8 +63,16 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm = null;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (PipelineNotFoundException pnfe) {
Review comment:
From SCMClient, I don't think we get PipelineNotFoundException I believe we get exception as IOException.
```
public static IOException getRemoteException(ServiceException se) {
Throwable e = se.getCause();
if (e == null) {
return new IOException(se);
}
return e instanceof IOException ? (IOException) e : new IOException(se);
}
```
```
private ScmContainerLocationResponse submitRequest(
StorageContainerLocationProtocolProtos.Type type,
Consumer<Builder> builderConsumer) throws IOException {
final ScmContainerLocationResponse response;
try {
Builder builder = ScmContainerLocationRequest.newBuilder()
.setCmdType(type)
.setVersion(CURRENT_VERSION)
.setTraceID(TracingUtil.exportCurrentSpan());
builderConsumer.accept(builder);
ScmContainerLocationRequest wrapper = builder.build();
response = submitRpcRequest(wrapper);
} catch (ServiceException ex) {
throw ProtobufHelper.getRemoteException(ex);
}
return response;
}
```
Can you add a simple test with a cluster start, and see do we get PipelineNotFoundException?
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658716156
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,8 +63,16 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm = null;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (PipelineNotFoundException pnfe) {
Review comment:
please see the log i mention [above ](https://github.com/apache/ozone/pull/2371#discussion_r658695134), the server side throw a PipelineNotFoundException, so if i do not misunderstand, the client side will also get a PipelineNotFoundException. please correct me if i am wrong
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r681573160
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,14 +64,28 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (IOException ex) {
+ if (ex instanceof RemoteException) {
+ IOException ioe = ((RemoteException) ex)
+ .unwrapRemoteException(PipelineNotFoundException.class);
+ if (ioe instanceof PipelineNotFoundException) {
Review comment:
In this case do we need to throw PipelineNotFoundException??
--
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 pull request #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#issuecomment-892386464
thanks @bharatviswa504 for the careful review, i merge the latest master branch to trigger CI again and wait for a clean CI before merge
--
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658714135
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java
##########
@@ -70,6 +71,9 @@ private HddsClientUtils() {
// Not Replicated Exception will be thrown if watch For commit
// does not succeed
.add(NotReplicatedException.class)
+ // for reconPipelineReportHandler , if could not find pipeline
Review comment:
do you mean if i want to forbid this retry here for pipelineNotFoundException , i should set it here?
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658651006
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineReportHandler.java
##########
@@ -93,6 +93,9 @@ public void onMessage(PipelineReportFromDatanode pipelineReportFromDatanode,
} catch(NotLeaderException ex) {
// Avoid NotLeaderException logging which happens when processing
// pipeline report on followers.
+ } catch(PipelineNotFoundException pnfe) {
Review comment:
Do you mean hadoop rpc which uses failover proxy provider i believe
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r678806819
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,13 +64,17 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm = null;
+ Pipeline pipelineFromScm;
try {
pipelineFromScm =
scmServiceProvider.getPipeline(report.getPipelineID());
- } catch (PipelineNotFoundException pnfe) {
- LOG.error("Could not find pipeline at SCM: {}.", pipelineID, pnfe);
+ } catch (IOException ex) {
+ assert (ex instanceof RemoteException);
+ IOException exception = ((RemoteException) ex)
+ .unwrapRemoteException(PipelineNotFoundException.class);
+ assert (exception instanceof PipelineNotFoundException);
Review comment:
@bharatviswa504 PTAL, thanks!
--
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658613549
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineReportHandler.java
##########
@@ -93,6 +93,9 @@ public void onMessage(PipelineReportFromDatanode pipelineReportFromDatanode,
} catch(NotLeaderException ex) {
// Avoid NotLeaderException logging which happens when processing
// pipeline report on followers.
+ } catch(PipelineNotFoundException pnfe) {
Review comment:
what i want to do here is to handle this
```
Pipeline pipelineFromScm =
scmServiceProvider.getPipeline(report.getPipelineID());
```
here , a scmclient is used to confirm the pipeline existence to scm, and this will take a grpc call.
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658695134
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineReportHandler.java
##########
@@ -93,6 +93,9 @@ public void onMessage(PipelineReportFromDatanode pipelineReportFromDatanode,
} catch(NotLeaderException ex) {
// Avoid NotLeaderException logging which happens when processing
// pipeline report on followers.
+ } catch(PipelineNotFoundException pnfe) {
Review comment:
yea, it seems use , please take a look at [this](https://issues.apache.org/jira/secure/attachment/13027279/%E6%88%AA%E5%B1%8F2021-06-25%20%E4%B8%8B%E5%8D%884.18.35.png)
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658605496
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineReportHandler.java
##########
@@ -93,6 +93,9 @@ public void onMessage(PipelineReportFromDatanode pipelineReportFromDatanode,
} catch(NotLeaderException ex) {
// Avoid NotLeaderException logging which happens when processing
// pipeline report on followers.
+ } catch(PipelineNotFoundException pnfe) {
Review comment:
Does nt recon pipelineHandler processPipelineReport handled this already?
ReconPipelineReportHandler.java
```
Pipeline pipeline = null;
try {
pipeline = reconPipelineManager.getPipeline(pipelineID);
} catch (PipelineNotFoundException ex) {
LOG.warn("Pipeline {} not found in Recon.", pipelineID);
return;
}
````
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java
##########
@@ -70,6 +71,9 @@ private HddsClientUtils() {
// Not Replicated Exception will be thrown if watch For commit
// does not succeed
.add(NotReplicatedException.class)
+ // for reconPipelineReportHandler , if could not find pipeline
Review comment:
And also I see this list is being used in KeyOutputStream and XceiverClientRatis.
Not understood why we need this to be added here.
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r674626962
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,8 +63,16 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm = null;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (PipelineNotFoundException pnfe) {
Review comment:
thanks for the review , i will fix this
--
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658626456
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java
##########
@@ -70,6 +71,9 @@ private HddsClientUtils() {
// Not Replicated Exception will be thrown if watch For commit
// does not succeed
.add(NotReplicatedException.class)
+ // for reconPipelineReportHandler , if could not find pipeline
Review comment:
when a scmclient is taking a grpc call, if any exception in this list thrown, it will stop retry, or else it will try for a maxcount at a specified interval , which are set at `SCMClientConfig.java`. here , i want `scmclient.getpipeline()` not to retry if PipelineNotFoundException is threw.
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r672971961
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,8 +63,16 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm = null;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (PipelineNotFoundException pnfe) {
Review comment:
````
try {
cluster.getStorageContainerLocationClient().getPipeline(HddsProtos.PipelineID.newBuilder().setId(UUID.randomUUID().toString()).build());
} catch (IOException ex) {
Assert.assertTrue(ex instanceof RemoteException);
PipelineNotFoundException exception = (PipelineNotFoundException) ((RemoteException) ex).unwrapRemoteException(PipelineNotFoundException.class);
Assert.assertTrue(exception instanceof PipelineNotFoundException);
}
````
We donot get PipelineNotFoundException, we get RemoteException wrapped with PipelineNotFoundException. We need to unwrap and check if it is PipelineNotFoundException.
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,8 +63,16 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm = null;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (PipelineNotFoundException pnfe) {
Review comment:
````
try {
cluster.getStorageContainerLocationClient().getPipeline(HddsProtos.PipelineID.newBuilder().setId(UUID.randomUUID().toString()).build());
} catch (IOException ex) {
Assert.assertTrue(ex instanceof RemoteException);
PipelineNotFoundException exception = (PipelineNotFoundException) ((RemoteException) ex).unwrapRemoteException(PipelineNotFoundException.class);
Assert.assertTrue(exception instanceof PipelineNotFoundException);
}
````
You can use this to check by updating test TestStorageContainerManagerHA#testAllSCMAreRunning. (During this found issue in getStorageContainerLocationClient() comment below code.
/* InetSocketAddress address = scm.getClientRpcAddress();*/
/* LOG.info(
"Creating StorageContainerLocationProtocol RPC client with address {}",
address);*/
We donot get PipelineNotFoundException, we get RemoteException wrapped with PipelineNotFoundException. We need to unwrap and check if it is PipelineNotFoundException.
--
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658654635
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,8 +63,16 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm = null;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (PipelineNotFoundException pnfe) {
Review comment:
From SCMClient, I don't think we get PipelineNotFoundException I believe we get exception as IOException.
```
public static IOException getRemoteException(ServiceException se) {
Throwable e = se.getCause();
if (e == null) {
return new IOException(se);
}
return e instanceof IOException ? (IOException) e : new IOException(se);
}
```
```
private ScmContainerLocationResponse submitRequest(
StorageContainerLocationProtocolProtos.Type type,
Consumer<Builder> builderConsumer) throws IOException {
final ScmContainerLocationResponse response;
try {
Builder builder = ScmContainerLocationRequest.newBuilder()
.setCmdType(type)
.setVersion(CURRENT_VERSION)
.setTraceID(TracingUtil.exportCurrentSpan());
builderConsumer.accept(builder);
ScmContainerLocationRequest wrapper = builder.build();
response = submitRpcRequest(wrapper);
} catch (ServiceException ex) {
throw ProtobufHelper.getRemoteException(ex);
}
return response;
}
```
Can you add a simple test with a cluster start, and see do we get PipelineNotFoundException when getPipeline with a pipeline does not exist?
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r678130218
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,13 +64,17 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm = null;
+ Pipeline pipelineFromScm;
try {
pipelineFromScm =
scmServiceProvider.getPipeline(report.getPipelineID());
- } catch (PipelineNotFoundException pnfe) {
- LOG.error("Could not find pipeline at SCM: {}.", pipelineID, pnfe);
+ } catch (IOException ex) {
+ assert (ex instanceof RemoteException);
+ IOException exception = ((RemoteException) ex)
+ .unwrapRemoteException(PipelineNotFoundException.class);
+ assert (exception instanceof PipelineNotFoundException);
Review comment:
Not understood why we need this assert here. It can also be any other exception right? (Lets say when SCM's are down)
--
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r682266330
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,14 +64,28 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm =
- scmServiceProvider.getPipeline(report.getPipelineID());
+ Pipeline pipelineFromScm;
+
+ try {
+ pipelineFromScm =
+ scmServiceProvider.getPipeline(report.getPipelineID());
+ } catch (IOException ex) {
+ if (ex instanceof RemoteException) {
+ IOException ioe = ((RemoteException) ex)
+ .unwrapRemoteException(PipelineNotFoundException.class);
+ if (ioe instanceof PipelineNotFoundException) {
Review comment:
thank for the review , i have updated the PR , please take a look
--
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658695134
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/PipelineReportHandler.java
##########
@@ -93,6 +93,9 @@ public void onMessage(PipelineReportFromDatanode pipelineReportFromDatanode,
} catch(NotLeaderException ex) {
// Avoid NotLeaderException logging which happens when processing
// pipeline report on followers.
+ } catch(PipelineNotFoundException pnfe) {
Review comment:
yea, it will use , please take a look at [this](https://issues.apache.org/jira/secure/attachment/13027279/%E6%88%AA%E5%B1%8F2021-06-25%20%E4%B8%8B%E5%8D%884.18.35.png)
--
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:
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #2371:
URL: https://github.com/apache/ozone/pull/2371
--
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 #2371: HDDS-5390. reconPipelineReportHandler should not retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r678793722
##########
File path: hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconPipelineReportHandler.java
##########
@@ -63,13 +64,17 @@ protected void processPipelineReport(PipelineReport report,
PipelineID pipelineID = PipelineID.getFromProtobuf(report.getPipelineID());
if (!reconPipelineManager.containsPipeline(pipelineID)) {
LOG.info("Unknown pipeline {}. Trying to get from SCM.", pipelineID);
- Pipeline pipelineFromScm = null;
+ Pipeline pipelineFromScm;
try {
pipelineFromScm =
scmServiceProvider.getPipeline(report.getPipelineID());
- } catch (PipelineNotFoundException pnfe) {
- LOG.error("Could not find pipeline at SCM: {}.", pipelineID, pnfe);
+ } catch (IOException ex) {
+ assert (ex instanceof RemoteException);
+ IOException exception = ((RemoteException) ex)
+ .unwrapRemoteException(PipelineNotFoundException.class);
+ assert (exception instanceof PipelineNotFoundException);
Review comment:
yea, thanks , i made a mistake here, i will change this
--
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 #2371: HDDS-5390. reconPipelineReportHandler don't retry when pipeline not found
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2371:
URL: https://github.com/apache/ozone/pull/2371#discussion_r658651701
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java
##########
@@ -70,6 +71,9 @@ private HddsClientUtils() {
// Not Replicated Exception will be thrown if watch For commit
// does not succeed
.add(NotReplicatedException.class)
+ // for reconPipelineReportHandler , if could not find pipeline
Review comment:
And also we don't use this, we use the list specified in
```
private static final List<Class<? extends Exception>>
NON_RETRIABLE_EXCEPTION_LIST =
ImmutableList.<Class<? extends Exception>>builder()
.add(SCMException.class)
.add(NonRetriableException.class)
.build();
```
SCMHAUtils.java
And also you can refer SCMHAUtils#getRetryAction logic for this.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org