sodonnel commented on code in PR #6127:
URL: https://github.com/apache/ozone/pull/6127#discussion_r1471586887
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -749,14 +749,10 @@ ContainerCommandResponseProto handleReadChunk(
@VisibleForTesting
void checkContainerIsHealthy(KeyValueContainer kvContainer, BlockID blockID,
Type cmd) {
- kvContainer.readLock();
- try {
- if (kvContainer.getContainerData().getState() == State.UNHEALTHY) {
- LOG.warn("{} request {} for UNHEALTHY container {} replica", cmd,
- blockID, kvContainer.getContainerData().getContainerID());
- }
- } finally {
- kvContainer.readUnlock();
+ // No kvContainer.readLock() for performance optimization
+ if (kvContainer.getContainerData().getState() == State.UNHEALTHY) {
Review Comment:
I don't think removing this lock will help much because:
`isContainerHealthy()` calls `kvContainer.getContainerData().getState()`
which looks like:
```
public synchronized ContainerDataProto.State getState() {
return state;
}
```
Which is actually worse than the read lock, as the synchronized method
prevents even concurrent readers. Even after removing this read lock, you will
need to deal with the synchronized method. There are quite a few methods on
that class which are synchronized.
All this method is doing is logging a message if the container is unhealthy
- do we need to do that? The method is called in only 4 places, always in this
sort of pattern:
```
try {
BlockID blockID = BlockID.getFromProtobuf(
request.getGetBlock().getBlockID());
checkContainerIsHealthy(kvContainer, blockID, Type.GetBlock);
responseData = blockManager.getBlock(kvContainer, blockID)
.getProtoBufMessage();
final long numBytes = responseData.getSerializedSize();
metrics.incContainerBytesStats(Type.GetBlock, numBytes);
} catch (StorageContainerException ex) {
return ContainerUtils.logAndReturnError(LOG, ex, request);
} catch (IOException ex) {
return ContainerUtils.logAndReturnError(LOG,
new StorageContainerException("Get Key failed", ex, IO_EXCEPTION),
request);
}
return getBlockDataResponse(request, responseData);
}
```
If the container is unhealthy, as we don't get an error on the read, do we
really care if it is unhealthy? Perhaps checkContainerHealthy could be moved
into the exception block to log only if there problems?
Another question is - does `ContainerData.getState()` even need to be
synchronized? As I understand it, updating a reference in Java is atomic - and
the state is just a reference which can be changed by `updateState()` - perhaps
it is fine for getState() to not be synchronized.
--
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]
