Skip to content

HDDS-8010. Improve DN warning message when getBlock does not find the block #4350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
GAOJHIHCYUAN wants to merge 5 commits into from
Closed

HDDS-8010. Improve DN warning message when getBlock does not find the block #4350

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab78e1f
HDDS-8010 Improve DN warning message when getBlock does not find the …
GAOJHIHCYUAN Mar 2, 2023
9928001
HDDS-8010.Improve DN warning message when getBlock does not find the …
GAOJHIHCYUAN Mar 5, 2023
21fd4ec
HDDS-8010. Improve DN warning message when getBlock does not find the…
GAOJHIHCYUAN Mar 6, 2023
2d7be68
Merge remote-tracking branch 'origin/HDDS-8010' into HDDS-8010
GAOJHIHCYUAN Mar 6, 2023
9c124b7
HDDS-8010. Improve DN warning message when getBlock does not find the…
GAOJHIHCYUAN Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions ...ervice/src/main/java/org/apache/hadoop/ozone/container/common/helpers/ContainerUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,18 @@ private ContainerUtils() {
public static ContainerCommandResponseProto logAndReturnError(
Logger log, StorageContainerException ex,
ContainerCommandRequestProto request) {
String logInfo = "Operation: {} , Trace ID: {} , Message: {} , " +
"Result: {} , StorageContainerException Occurred.";
String logInfo = "Operation: {} , Trace ID: {} , Block ID: {} , " +
"Message: {} , Result: {} , StorageContainerException Occurred.";
if (ex.getResult() == CLOSED_CONTAINER_IO ||
ex.getResult() == CONTAINER_NOT_OPEN) {
if (log.isDebugEnabled()) {
log.debug(logInfo, request.getCmdType(), request.getTraceID(),
request.getGetBlock().getBlockID(),
Copy link
Contributor

@chungen0126 chungen0126 Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be still logged at DEBUG level if it is under the isDebugEnabled() condition.

Copy link
Contributor Author

@GAOJHIHCYUAN GAOJHIHCYUAN Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your comment
I commit new version that if it is under the condition still logged at DEBUG level.
if it's not under the condition logged at Warn level.

ex.getMessage(), ex.getResult().getValueDescriptor().getName(), ex);
}
} else {
log.info(logInfo, request.getCmdType(), request.getTraceID(),
log.warn(logInfo, request.getCmdType(), request.getTraceID(),
request.getGetBlock().getBlockID(),
Copy link
Contributor

@adoroszlai adoroszlai Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request.getGetBlock() is only applicable to GetBlock requests. Also, not all commands are related to blocks anyway.

Looking at HDDS-8010, I think the goal is to change the message in:

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java

Lines 353 to 364 in 4afa68e

private BlockData getBlockByID(DBHandle db, BlockID blockID,
KeyValueContainerData containerData) throws IOException {
String blockKey = containerData.getBlockKey(blockID.getLocalID());
BlockData blockData = db.getStore().getBlockDataTable().get(blockKey);
if (blockData == null) {
throw new StorageContainerException(NO_SUCH_BLOCK_ERR_MSG,
NO_SUCH_BLOCK);
}
return blockData;
}

Copy link
Contributor Author

@GAOJHIHCYUAN GAOJHIHCYUAN Mar 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request.getGetBlock() is only applicable to GetBlock requests. Also, not all commands are related to blocks anyway.

Looking at HDDS-8010, I think the goal is to change the message in:

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/BlockManagerImpl.java

Lines 353 to 364 in 4afa68e

private BlockData getBlockByID(DBHandle db, BlockID blockID,
KeyValueContainerData containerData) throws IOException {
String blockKey = containerData.getBlockKey(blockID.getLocalID());
BlockData blockData = db.getStore().getBlockDataTable().get(blockKey);
if (blockData == null) {
throw new StorageContainerException(NO_SUCH_BLOCK_ERR_MSG,
NO_SUCH_BLOCK);
}
return blockData;
} 

public class StorageContainerException extends IOException {
  private final ContainerProtos.Result result;

  private final BlockID blockID = getLocalID();

  public BlockID getLocalID() {return blockID;}

if (log.isDebugEnabled()) {
        log.debug(logInfo, request.getCmdType(), request.getTraceID(),
            ex.getLocalID(),
            ex.getMessage(), ex.getResult().getValueDescriptor().getName(), ex);
      }
    } else {
      log.warn(logInfo, request.getCmdType(), request.getTraceID(),
          ex.getLocalID(),
          ex.getMessage(), ex.getResult().getValueDescriptor().getName(), ex);
    }

because the log was logged when StorageContainerException occurred
my opinion was adding the getLocalID() in StorageContainerException.java
and then we can call function in getBlockByID() getting the block ID.
Idk is it a good soultion.
thanks you so much

@GAOJHIHCYUAN have you observed the updated message in the log?

No,I haven't
I will try how could I observe the updated message.
sorry,i was a newbie. I'm still learning.
thanks you so much.

Copy link
Contributor

@ashishkumar50 ashishkumar50 Apr 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can update message in getBlockByID while throwing StorageContainerException to print the blockID. Can you update the PR?

Copy link
Contributor Author

@GAOJHIHCYUAN GAOJHIHCYUAN Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can update message in getBlockByID while throwing StorageContainerException to print the blockID. Can you update the PR?

okay,I will update the PR.
appreciate

ex.getMessage(), ex.getResult().getValueDescriptor().getName(), ex);
}
return getContainerCommandResponse(request, ex.getResult(), ex.getMessage())
Expand Down