You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "jojochuang (via GitHub)" <gi...@apache.org> on 2024/03/26 19:56:21 UTC
[PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
jojochuang opened a new pull request, #6444:
URL: https://github.com/apache/ozone/pull/6444
## What changes were proposed in this pull request?
HDDS-10591. [hsync] improve block token refresh message.
Please describe your PR in detail:
Usually an expired block token received by a DataNode is refreshed properly. However the relevant log messages look scary and confusing.
* Some of the logs should tone down. Instead of ERROR, it should be a WARN, because the exception will be retried.
* The log messages surrounding block token refresh should be more clear.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10591
## How was this patch tested?
Log message only. Existing unit tests should suffice.
--
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
Re: [PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #6444:
URL: https://github.com/apache/ozone/pull/6444#discussion_r1550172945
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -217,18 +218,25 @@ private boolean isConnectivityIssue(IOException ex) {
}
private void refreshBlockInfo(IOException cause) throws IOException {
- LOG.info("Unable to read information for block {} from pipeline {}: {}",
+ LOG.info("Attempting to update pipeline and block token for block {} from pipeline {}: {}",
blockID, pipelineRef.get().getId(), cause.getMessage());
if (refreshFunction != null) {
LOG.debug("Re-fetching pipeline and block token for block {}", blockID);
BlockLocationInfo blockLocationInfo = refreshFunction.apply(blockID);
if (blockLocationInfo == null) {
- LOG.debug("No new block location info for block {}", blockID);
+ LOG.warn("No new block location info for block {}", blockID);
} else {
- LOG.debug("New pipeline for block {}: {}", blockID,
- blockLocationInfo.getPipeline());
setPipeline(blockLocationInfo.getPipeline());
- tokenRef.set(blockLocationInfo.getToken());
+ LOG.info("New pipeline for block {}: {}", blockID,
+ blockLocationInfo.getPipeline());
+
+ if (blockLocationInfo.getToken() != null) {
+ tokenRef.set(blockLocationInfo.getToken());
Review Comment:
When would `blockLocationInfo.getToken()` be null inside `refreshBlockInfo()` call 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.
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
Re: [PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #6444:
URL: https://github.com/apache/ozone/pull/6444#discussion_r1548255681
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -217,18 +218,27 @@ private boolean isConnectivityIssue(IOException ex) {
}
private void refreshBlockInfo(IOException cause) throws IOException {
- LOG.info("Unable to read information for block {} from pipeline {}: {}",
+ LOG.info("Attempting to update pipeline and block token for block {} from pipeline {}: {}",
blockID, pipelineRef.get().getId(), cause.getMessage());
if (refreshFunction != null) {
LOG.debug("Re-fetching pipeline and block token for block {}", blockID);
BlockLocationInfo blockLocationInfo = refreshFunction.apply(blockID);
if (blockLocationInfo == null) {
- LOG.debug("No new block location info for block {}", blockID);
+ LOG.warn("No new block location info for block {}", blockID);
} else {
- LOG.debug("New pipeline for block {}: {}", blockID,
- blockLocationInfo.getPipeline());
setPipeline(blockLocationInfo.getPipeline());
- tokenRef.set(blockLocationInfo.getToken());
+ LOG.info("New pipeline for block {}: {}", blockID,
+ blockLocationInfo.getPipeline());
+
+ if (blockLocationInfo.getToken() != null) {
+ tokenRef.set(blockLocationInfo.getToken());
+ OzoneBlockTokenIdentifier tokenId = new OzoneBlockTokenIdentifier();
+ tokenId.readFromByteArray(tokenRef.get().getIdentifier());
+ LOG.info("A new token is added for block {}. Expiry: {}",
+ blockID, Instant.ofEpochMilli(tokenId.getExpiryDate()));
+ } else {
+ LOG.warn("Unable to add a new block token for block {}", blockID);
Review Comment:
Oh yeah you're right. Good catch.
--
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
Re: [PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #6444:
URL: https://github.com/apache/ozone/pull/6444#discussion_r1546717403
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -217,18 +217,19 @@ private boolean isConnectivityIssue(IOException ex) {
}
private void refreshBlockInfo(IOException cause) throws IOException {
- LOG.info("Unable to read information for block {} from pipeline {}: {}",
+ LOG.info("Attempting to update pipeline and block token for block {} from pipeline {}: {}",
blockID, pipelineRef.get().getId(), cause.getMessage());
if (refreshFunction != null) {
LOG.debug("Re-fetching pipeline and block token for block {}", blockID);
BlockLocationInfo blockLocationInfo = refreshFunction.apply(blockID);
if (blockLocationInfo == null) {
LOG.debug("No new block location info for block {}", blockID);
} else {
- LOG.debug("New pipeline for block {}: {}", blockID,
- blockLocationInfo.getPipeline());
setPipeline(blockLocationInfo.getPipeline());
tokenRef.set(blockLocationInfo.getToken());
+ LOG.info("New pipeline for block {}: {}", blockID,
+ blockLocationInfo.getPipeline());
+ LOG.info("A new token is added {}", blockLocationInfo.getToken());
Review Comment:
How about logging the expiry time of the token?
--
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
Re: [PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang merged PR #6444:
URL: https://github.com/apache/ozone/pull/6444
--
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
Re: [PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #6444:
URL: https://github.com/apache/ozone/pull/6444#discussion_r1540554160
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -217,18 +217,19 @@ private boolean isConnectivityIssue(IOException ex) {
}
private void refreshBlockInfo(IOException cause) throws IOException {
- LOG.info("Unable to read information for block {} from pipeline {}: {}",
+ LOG.info("Attempting to update pipeline and block token for block {} from pipeline {}: {}",
blockID, pipelineRef.get().getId(), cause.getMessage());
if (refreshFunction != null) {
LOG.debug("Re-fetching pipeline and block token for block {}", blockID);
BlockLocationInfo blockLocationInfo = refreshFunction.apply(blockID);
if (blockLocationInfo == null) {
LOG.debug("No new block location info for block {}", blockID);
} else {
- LOG.debug("New pipeline for block {}: {}", blockID,
- blockLocationInfo.getPipeline());
setPipeline(blockLocationInfo.getPipeline());
tokenRef.set(blockLocationInfo.getToken());
+ LOG.info("New pipeline for block {}: {}", blockID,
+ blockLocationInfo.getPipeline());
+ LOG.info("A new token is added {}", blockLocationInfo.getToken());
Review Comment:
It's better not log the token as it's a sensitive information.
--
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
Re: [PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #6444:
URL: https://github.com/apache/ozone/pull/6444#discussion_r1551992162
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -217,18 +218,25 @@ private boolean isConnectivityIssue(IOException ex) {
}
private void refreshBlockInfo(IOException cause) throws IOException {
- LOG.info("Unable to read information for block {} from pipeline {}: {}",
+ LOG.info("Attempting to update pipeline and block token for block {} from pipeline {}: {}",
blockID, pipelineRef.get().getId(), cause.getMessage());
if (refreshFunction != null) {
LOG.debug("Re-fetching pipeline and block token for block {}", blockID);
BlockLocationInfo blockLocationInfo = refreshFunction.apply(blockID);
if (blockLocationInfo == null) {
- LOG.debug("No new block location info for block {}", blockID);
+ LOG.warn("No new block location info for block {}", blockID);
} else {
- LOG.debug("New pipeline for block {}: {}", blockID,
- blockLocationInfo.getPipeline());
setPipeline(blockLocationInfo.getPipeline());
- tokenRef.set(blockLocationInfo.getToken());
+ LOG.info("New pipeline for block {}: {}", blockID,
+ blockLocationInfo.getPipeline());
+
+ if (blockLocationInfo.getToken() != null) {
+ tokenRef.set(blockLocationInfo.getToken());
Review Comment:
Then would it be better to place `tokenRef.set(blockLocationInfo.getToken());` before `if` just like before? To allow `tokenRef.set(null)` to happen just in case (though unlikely)?
```suggestion
tokenRef.set(blockLocationInfo.getToken());
if (blockLocationInfo.getToken() != 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
Re: [PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #6444:
URL: https://github.com/apache/ozone/pull/6444#issuecomment-2040377025
Merged. Thanks Siyao and Sammi
--
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
Re: [PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #6444:
URL: https://github.com/apache/ozone/pull/6444#discussion_r1550183032
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -217,18 +218,25 @@ private boolean isConnectivityIssue(IOException ex) {
}
private void refreshBlockInfo(IOException cause) throws IOException {
- LOG.info("Unable to read information for block {} from pipeline {}: {}",
+ LOG.info("Attempting to update pipeline and block token for block {} from pipeline {}: {}",
blockID, pipelineRef.get().getId(), cause.getMessage());
if (refreshFunction != null) {
LOG.debug("Re-fetching pipeline and block token for block {}", blockID);
BlockLocationInfo blockLocationInfo = refreshFunction.apply(blockID);
if (blockLocationInfo == null) {
- LOG.debug("No new block location info for block {}", blockID);
+ LOG.warn("No new block location info for block {}", blockID);
} else {
- LOG.debug("New pipeline for block {}: {}", blockID,
- blockLocationInfo.getPipeline());
setPipeline(blockLocationInfo.getPipeline());
- tokenRef.set(blockLocationInfo.getToken());
+ LOG.info("New pipeline for block {}: {}", blockID,
+ blockLocationInfo.getPipeline());
+
+ if (blockLocationInfo.getToken() != null) {
+ tokenRef.set(blockLocationInfo.getToken());
Review Comment:
possible in unsecure environment where block token is disabled.
--
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
Re: [PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #6444:
URL: https://github.com/apache/ozone/pull/6444#discussion_r1552142067
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -217,18 +218,25 @@ private boolean isConnectivityIssue(IOException ex) {
}
private void refreshBlockInfo(IOException cause) throws IOException {
- LOG.info("Unable to read information for block {} from pipeline {}: {}",
+ LOG.info("Attempting to update pipeline and block token for block {} from pipeline {}: {}",
blockID, pipelineRef.get().getId(), cause.getMessage());
if (refreshFunction != null) {
LOG.debug("Re-fetching pipeline and block token for block {}", blockID);
BlockLocationInfo blockLocationInfo = refreshFunction.apply(blockID);
if (blockLocationInfo == null) {
- LOG.debug("No new block location info for block {}", blockID);
+ LOG.warn("No new block location info for block {}", blockID);
} else {
- LOG.debug("New pipeline for block {}: {}", blockID,
- blockLocationInfo.getPipeline());
setPipeline(blockLocationInfo.getPipeline());
- tokenRef.set(blockLocationInfo.getToken());
+ LOG.info("New pipeline for block {}: {}", blockID,
+ blockLocationInfo.getPipeline());
+
+ if (blockLocationInfo.getToken() != null) {
+ tokenRef.set(blockLocationInfo.getToken());
Review Comment:
I'm fine either way. blockLocationInfo.getToken() should only be null if block token is disabled. In which case, tokenRef would have been null too already.
--
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
Re: [PR] HDDS-10591. [hsync] improve block token refresh message. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #6444:
URL: https://github.com/apache/ozone/pull/6444#discussion_r1547258300
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java:
##########
@@ -217,18 +218,27 @@ private boolean isConnectivityIssue(IOException ex) {
}
private void refreshBlockInfo(IOException cause) throws IOException {
- LOG.info("Unable to read information for block {} from pipeline {}: {}",
+ LOG.info("Attempting to update pipeline and block token for block {} from pipeline {}: {}",
blockID, pipelineRef.get().getId(), cause.getMessage());
if (refreshFunction != null) {
LOG.debug("Re-fetching pipeline and block token for block {}", blockID);
BlockLocationInfo blockLocationInfo = refreshFunction.apply(blockID);
if (blockLocationInfo == null) {
- LOG.debug("No new block location info for block {}", blockID);
+ LOG.warn("No new block location info for block {}", blockID);
} else {
- LOG.debug("New pipeline for block {}: {}", blockID,
- blockLocationInfo.getPipeline());
setPipeline(blockLocationInfo.getPipeline());
- tokenRef.set(blockLocationInfo.getToken());
+ LOG.info("New pipeline for block {}: {}", blockID,
+ blockLocationInfo.getPipeline());
+
+ if (blockLocationInfo.getToken() != null) {
+ tokenRef.set(blockLocationInfo.getToken());
+ OzoneBlockTokenIdentifier tokenId = new OzoneBlockTokenIdentifier();
+ tokenId.readFromByteArray(tokenRef.get().getIdentifier());
+ LOG.info("A new token is added for block {}. Expiry: {}",
+ blockID, Instant.ofEpochMilli(tokenId.getExpiryDate()));
+ } else {
+ LOG.warn("Unable to add a new block token for block {}", blockID);
Review Comment:
If security is not enabled, then token will be null, this WARN log will be confusing, I would suggest remove 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