You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "whbing (via GitHub)" <gi...@apache.org> on 2024/01/30 13:51:00 UTC
[PR] HDDS-10246. Remove readLock from KeyValueHandler.checkContainerIsHealthy [ozone]
whbing opened a new pull request, #6127:
URL: https://github.com/apache/ozone/pull/6127
## What changes were proposed in this pull request?
As described in HDDS-10146, all of the ChunkReader threads are blocked in the `KeyValueHandler#checkContainerIsHealthy` method, waiting for the `KeyValueContainer#readLock`.
Remove readLock from KeyValueHandler.checkContainerIsHealthy to improve performance
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10246
## How was this patch tested?
- No new test and pass exist unit tests
- Works well in our cluster
--
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-10246. Remove readLock from KeyValueHandler.checkContainerIsHealthy [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #6127:
URL: https://github.com/apache/ozone/pull/6127#discussion_r1471415088
##########
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:
@sodonnel , you can find the detail info under HDDS-10146. The scenario is when there are a lot of block deletions happening, read other blocks in the same container as being deleted block will be blocked due to unable to query container's read lock. The lock is hold by the thread doing the block deletion.
--
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-10246. Remove KeyValueHandler.checkContainerIsHealthy to improve read performance [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi merged PR #6127:
URL: https://github.com/apache/ozone/pull/6127
--
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-10246. Remove readLock from KeyValueHandler.checkContainerIsHealthy [ozone]
Posted by "whbing (via GitHub)" <gi...@apache.org>.
whbing commented on PR #6127:
URL: https://github.com/apache/ozone/pull/6127#issuecomment-1918912013
> as this `getState ` is fully synchronized method. We need to see if this method is really needed to be `synchronized`. I feel it is not need to be `synchronized`.
Not sure to what extent it affects performance. At least in practice, we haven't encountered any bottlenecks caused by `synchronized getState` yet. Maybe we can revisit it in the future if the problem occurs ?
--
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-10246. Remove readLock from KeyValueHandler.checkContainerIsHealthy [ozone]
Posted by "whbing (via GitHub)" <gi...@apache.org>.
whbing commented on code in PR #6127:
URL: https://github.com/apache/ozone/pull/6127#discussion_r1471408463
##########
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:
https://github.com/apache/ozone/blob/bd1cf6844a083515f8927c5df9b7b2642e3cf183/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingTask.java#L139-L143
`BlockDeletingTask#handleDeleteTask` will hold `KeyValueContainer.writeLock()`, and sometimes it takes a long time, as follow logs:
```java
2024-01-04 16:30:16,752 [BlockDeletingService#8] INFO org.apache.hadoop.ozone.container.keyvalue.statemachine.background.BlockDeletingTask: Max lock hold time (1000 ms) reached after 1390 ms. Completed 1 transactions, deleted 0 blocks. In container: 5455053. Releasing lock and resuming deletion later.
2024-01-04 17:01:16,897 [BlockDeletingService#1] INFO org.apache.hadoop.ozone.container.keyvalue.statemachine.background.BlockDeletingTask: Max lock hold time (1000 ms) reached after 1292 ms. Completed 1 transactions, deleted 0 blocks. In container: 5458979. Releasing lock and resuming deletion later.
```
This will result in a long waiting when the data in this container is read.
--
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-10246. Remove readLock from KeyValueHandler.checkContainerIsHealthy [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #6127:
URL: https://github.com/apache/ozone/pull/6127#discussion_r1471348176
##########
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:
Are you seeing performance problems when the container is unhealthy, along with a lot of log messages?
This lock is on a container, not "all containers at the same time", so do you also have a very high volume of requests for the same block?
At the very least, we should not log under a lock, as logging can be slow, but I would like to understand more about the problem before just removing the lock. On first look, it does not seem like it should be a problem.
--
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-10246. Remove readLock from KeyValueHandler.checkContainerIsHealthy [ozone]
Posted by "whbing (via GitHub)" <gi...@apache.org>.
whbing commented on code in PR #6127:
URL: https://github.com/apache/ozone/pull/6127#discussion_r1472578100
##########
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:
@ChenSammi @sodonnel Updated commit with just removing `checkContainerIsHealthy` method and the 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.
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-10246. Remove readLock from KeyValueHandler.checkContainerIsHealthy [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
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: 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-10246. Remove readLock from KeyValueHandler.checkContainerIsHealthy [ozone]
Posted by "whbing (via GitHub)" <gi...@apache.org>.
whbing commented on PR #6127:
URL: https://github.com/apache/ozone/pull/6127#issuecomment-1916901013
@ChenSammi 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
Re: [PR] HDDS-10246. Remove readLock from KeyValueHandler.checkContainerIsHealthy [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #6127:
URL: https://github.com/apache/ozone/pull/6127#discussion_r1472542310
##########
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 realized after I wrote the above, the the synchronization on the getter is probably not an issue. The problem root cause is the write lock being held for a long time by the delete thread. It would still be best to avoid the synchronization too if we can just remove the call entirely. The log message doesn't add much value compare to the cost of it via serialization.
--
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