You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "duongkame (via GitHub)" <gi...@apache.org> on 2024/08/08 00:59:58 UTC
[PR] HDDS-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
duongkame opened a new pull request, #7047:
URL: https://github.com/apache/ozone/pull/7047
## What changes were proposed in this pull request?
Fix exception handling when calling hsync concurrently
## What is the link to the Apache JIRA
## How was this patch tested?
(Please explain how this patch was tested. Ex: unit tests, manual tests, workflow run on the fork git repo.)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove 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
Re: [PR] HDDS-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1724078709
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java:
##########
@@ -581,7 +581,10 @@ private CompletableFuture<Message> writeStateMachineData(
writeChunkFuture.thenApply(r -> {
if (r.getResult() != ContainerProtos.Result.SUCCESS
&& r.getResult() != ContainerProtos.Result.CONTAINER_NOT_OPEN
- && r.getResult() != ContainerProtos.Result.CLOSED_CONTAINER_IO) {
+ && r.getResult() != ContainerProtos.Result.CLOSED_CONTAINER_IO
+ // After concurrent flushes are allowed on the same key, chunk file inconsistencies can happen and
Review Comment:
Got it. This is fine. 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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1724676403
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestHSync.java:
##########
@@ -667,6 +675,99 @@ static void runTestHSync(FileSystem fs, Path file,
}, 500, 3000);
}
+
+ public static Stream<Arguments> concurrentExceptionHandling() {
+ return Stream.of(
+ Arguments.of(4, 1),
+ Arguments.of(4, 4),
+ Arguments.of(8, 4)
+ );
+ }
+
+ @ParameterizedTest
+ @MethodSource("concurrentExceptionHandling")
+ public void testConcurrentExceptionHandling(int syncerThreads, int errors) throws Exception {
+ final String rootPath = String.format("%s://%s/", OZONE_OFS_URI_SCHEME, CONF.get(OZONE_OM_ADDRESS_KEY));
+ CONF.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, rootPath);
+
+ ErrorInjectorImpl errorInjector = new ErrorInjectorImpl();
+ XceiverClientManager.enableErrorInjection(errorInjector);
+
+ final String dir = OZONE_ROOT + bucket.getVolumeName() + OZONE_URI_DELIMITER + bucket.getName();
+
+ try (FileSystem fs = FileSystem.get(CONF)) {
+ final Path file = new Path(dir, "exceptionhandling");
+ byte[] data = new byte[8];
+ ThreadLocalRandom.current().nextBytes(data);
+ int writes;
+ try (FSDataOutputStream out = fs.create(file, true)) {
+ writes = runConcurrentWriteHSyncWithException(file, out, data, syncerThreads, errors, errorInjector);
+ }
+ validateWrittenFile(file, fs, data, writes);
+ fs.delete(file, false);
Review Comment:
> seems to be failing consistently: TestHSync.testUncommittedBlocks:515 expected: but was:
>
> I wonder if it's related to the new commit just merged #7074
To address the failure in `TestHsync#testUncommittedBlocks`, ensure that the `deletedTable` is empty after `fs.delete(file, false)`;
Rebase to master and add a line after `fs.delete(file, false);` to wait for `deletedTable` cleaning up.
```
waitForEmptyDeletedTable();
```
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1724676403
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestHSync.java:
##########
@@ -667,6 +675,99 @@ static void runTestHSync(FileSystem fs, Path file,
}, 500, 3000);
}
+
+ public static Stream<Arguments> concurrentExceptionHandling() {
+ return Stream.of(
+ Arguments.of(4, 1),
+ Arguments.of(4, 4),
+ Arguments.of(8, 4)
+ );
+ }
+
+ @ParameterizedTest
+ @MethodSource("concurrentExceptionHandling")
+ public void testConcurrentExceptionHandling(int syncerThreads, int errors) throws Exception {
+ final String rootPath = String.format("%s://%s/", OZONE_OFS_URI_SCHEME, CONF.get(OZONE_OM_ADDRESS_KEY));
+ CONF.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, rootPath);
+
+ ErrorInjectorImpl errorInjector = new ErrorInjectorImpl();
+ XceiverClientManager.enableErrorInjection(errorInjector);
+
+ final String dir = OZONE_ROOT + bucket.getVolumeName() + OZONE_URI_DELIMITER + bucket.getName();
+
+ try (FileSystem fs = FileSystem.get(CONF)) {
+ final Path file = new Path(dir, "exceptionhandling");
+ byte[] data = new byte[8];
+ ThreadLocalRandom.current().nextBytes(data);
+ int writes;
+ try (FSDataOutputStream out = fs.create(file, true)) {
+ writes = runConcurrentWriteHSyncWithException(file, out, data, syncerThreads, errors, errorInjector);
+ }
+ validateWrittenFile(file, fs, data, writes);
+ fs.delete(file, false);
Review Comment:
> seems to be failing consistently: TestHSync.testUncommittedBlocks:515 expected: but was:
>
> I wonder if it's related to the new commit just merged #7074
To address the failure in `TestHsync#testUncommittedBlocks`, ensure that the `deletedTable` is empty after `fs.delete(file, false)`;
Rebase to master and add a line after `fs.delete(file, false);` to wait `KeyDeletingService` to clean up the `deletedTable`.
```
waitForEmptyDeletedTable();
```
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1722459892
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -406,41 +401,35 @@ private void updatePutBlockLength() {
* @throws IOException if error occurred
*/
- // In this case, the data is already cached in the currentBuffer.
+ // In this case, the data is already cached in the allocated buffers in the BufferPool.
public synchronized void writeOnRetry(long len) throws IOException {
if (len == 0) {
return;
}
+ List<ChunkBuffer> allocatedBuffers = bufferPool.getAllocatedBuffers();
if (LOG.isDebugEnabled()) {
- LOG.debug("Retrying write length {} for blockID {}", len, blockID);
+ LOG.debug("{}: Retrying write length {} on target blockID {}, {} buffers", this, len, blockID,
+ allocatedBuffers.size());
}
Preconditions.checkArgument(len <= streamBufferArgs.getStreamBufferMaxSize());
int count = 0;
- List<ChunkBuffer> allocatedBuffers = bufferPool.getAllocatedBuffers();
while (len > 0) {
ChunkBuffer buffer = allocatedBuffers.get(count);
long writeLen = Math.min(buffer.position(), len);
- if (!buffer.hasRemaining()) {
- writeChunk(buffer);
- }
len -= writeLen;
count++;
writtenDataLength += writeLen;
- // we should not call isBufferFull/shouldFlush here.
- // The buffer might already be full as whole data is already cached in
- // the buffer. We should just validate
- // if we wrote data of size streamBufferMaxSize/streamBufferFlushSize to
- // call for handling full buffer/flush buffer condition.
- if (writtenDataLength % streamBufferArgs.getStreamBufferFlushSize() == 0) {
- // reset the position to zero as now we will be reading the
- // next buffer in the list
- updateWriteChunkLength();
- updatePutBlockLength();
- CompletableFuture<PutBlockResult> putBlockResultFuture = executePutBlock(false, false);
- recordWatchForCommitAsync(putBlockResultFuture);
- }
- if (writtenDataLength == streamBufferArgs.getStreamBufferMaxSize()) {
- handleFullBuffer();
+ updateWriteChunkLength();
+ updatePutBlockLength();
+ LOG.debug("Write chunk on retry buffer = {}", buffer);
+ CompletableFuture<PutBlockResult> f = writeChunkAndPutBlock(buffer, false);
Review Comment:
writeChunkAndPutBlock() was initially used only when WriteChunk/PutBlock piggybacking is enabled.
This can cause compatibility problems if server side does not support piggybacking.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntry.java:
##########
@@ -102,6 +119,37 @@ void checkStream() throws IOException {
}
}
+ /** Register when a call (write or flush) is received on this block. */
+ void registerCallReceived() {
+ inflightCalls.incrementAndGet();
+ }
+
+ /**
+ * Register when a call (write or flush) is finised on this block.
Review Comment:
```suggestion
* Register when a call (write or flush) is finished on this block.
```
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntry.java:
##########
@@ -69,6 +73,18 @@ public class BlockOutputStreamEntry extends OutputStream {
private final StreamBufferArgs streamBufferArgs;
private final Supplier<ExecutorService> executorServiceSupplier;
+ /**
+ * An indicator that this BlockOutputStream is created to handoff writes from another faulty BlockOutputStream.
+ * Once this flag is on, this BlockOutputStream can only handle writeOneRetry.
Review Comment:
typo: writeOnRetry
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -290,8 +325,15 @@ private int writeToOutputStream(BlockOutputStreamEntry current,
* @param exception actual exception that occurred
* @throws IOException Throws IOException if Write fails
*/
- private synchronized void handleException(BlockOutputStreamEntry streamEntry,
- IOException exception) throws IOException {
+ private void handleExceptionInternal(BlockOutputStreamEntry streamEntry, IOException exception) throws IOException {
+ try {
+ // Wait for all pending flushes in the faulty stream. It's possible that a prior write is pending completion
+ // successfully. Errors are ignored here and will be handled by the individual flush call. We just wall to ensure
Review Comment:
```suggestion
// successfully. Errors are ignored here and will be handled by the individual flush call. We just want to ensure
```
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1720502509
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -91,7 +95,7 @@ enum StreamAction {
// how much of data is actually written yet to underlying stream
private long offset;
// how much data has been ingested into the stream
- private volatile long writeOffset;
+ private long writeOffset;
Review Comment:
I removed it accidentally, reverted.
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1722811012
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -406,41 +401,35 @@ private void updatePutBlockLength() {
* @throws IOException if error occurred
*/
- // In this case, the data is already cached in the currentBuffer.
+ // In this case, the data is already cached in the allocated buffers in the BufferPool.
public synchronized void writeOnRetry(long len) throws IOException {
if (len == 0) {
return;
}
+ List<ChunkBuffer> allocatedBuffers = bufferPool.getAllocatedBuffers();
if (LOG.isDebugEnabled()) {
- LOG.debug("Retrying write length {} for blockID {}", len, blockID);
+ LOG.debug("{}: Retrying write length {} on target blockID {}, {} buffers", this, len, blockID,
+ allocatedBuffers.size());
}
Preconditions.checkArgument(len <= streamBufferArgs.getStreamBufferMaxSize());
int count = 0;
- List<ChunkBuffer> allocatedBuffers = bufferPool.getAllocatedBuffers();
while (len > 0) {
ChunkBuffer buffer = allocatedBuffers.get(count);
long writeLen = Math.min(buffer.position(), len);
- if (!buffer.hasRemaining()) {
- writeChunk(buffer);
- }
len -= writeLen;
count++;
writtenDataLength += writeLen;
- // we should not call isBufferFull/shouldFlush here.
- // The buffer might already be full as whole data is already cached in
- // the buffer. We should just validate
- // if we wrote data of size streamBufferMaxSize/streamBufferFlushSize to
- // call for handling full buffer/flush buffer condition.
- if (writtenDataLength % streamBufferArgs.getStreamBufferFlushSize() == 0) {
- // reset the position to zero as now we will be reading the
- // next buffer in the list
- updateWriteChunkLength();
- updatePutBlockLength();
- CompletableFuture<PutBlockResult> putBlockResultFuture = executePutBlock(false, false);
- recordWatchForCommitAsync(putBlockResultFuture);
- }
- if (writtenDataLength == streamBufferArgs.getStreamBufferMaxSize()) {
- handleFullBuffer();
+ updateWriteChunkLength();
+ updatePutBlockLength();
+ LOG.debug("Write chunk on retry buffer = {}", buffer);
+ CompletableFuture<PutBlockResult> f = writeChunkAndPutBlock(buffer, false);
Review Comment:
Good point, we can only `writeChunkAndPutBlock` when piggybacking is enabled. Updated.
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #7047:
URL: https://github.com/apache/ozone/pull/7047#issuecomment-2302964220
Merged. Thanks @duongkame for the patch and @smengcl @chungen0126 for reviews!
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang merged PR #7047:
URL: https://github.com/apache/ozone/pull/7047
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on PR #7047:
URL: https://github.com/apache/ozone/pull/7047#issuecomment-2302910041
> seems to be failing consistently: TestHSync.testUncommittedBlocks:515 expected: but was:
>
> I wonder if it's related to the new commit just merged #7074
Got it. Fixed as @chungen0126 suggested.
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1724054498
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java:
##########
@@ -581,7 +581,10 @@ private CompletableFuture<Message> writeStateMachineData(
writeChunkFuture.thenApply(r -> {
if (r.getResult() != ContainerProtos.Result.SUCCESS
&& r.getResult() != ContainerProtos.Result.CONTAINER_NOT_OPEN
- && r.getResult() != ContainerProtos.Result.CLOSED_CONTAINER_IO) {
+ && r.getResult() != ContainerProtos.Result.CLOSED_CONTAINER_IO
+ // After concurrent flushes are allowed on the same key, chunk file inconsistencies can happen and
Review Comment:
Turns out it's a simple fix so I include it here. I can move it to a separate PR if neccessary.
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on PR #7047:
URL: https://github.com/apache/ozone/pull/7047#issuecomment-2299871500
Did a rebase and reran the repeating test.
https://github.com/duongkame/ozone/actions/runs/10475934264
https://github.com/duongkame/ozone/actions/runs/10475943780
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1724093379
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java:
##########
@@ -581,7 +581,10 @@ private CompletableFuture<Message> writeStateMachineData(
writeChunkFuture.thenApply(r -> {
if (r.getResult() != ContainerProtos.Result.SUCCESS
&& r.getResult() != ContainerProtos.Result.CONTAINER_NOT_OPEN
- && r.getResult() != ContainerProtos.Result.CLOSED_CONTAINER_IO) {
+ && r.getResult() != ContainerProtos.Result.CLOSED_CONTAINER_IO
+ // After concurrent flushes are allowed on the same key, chunk file inconsistencies can happen and
+ // that should now
Review Comment:
incomplete sentence?
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #7047:
URL: https://github.com/apache/ozone/pull/7047#issuecomment-2301279880
seems to be failing consistently: TestHSync.testUncommittedBlocks:515 expected: <true> but was: <false>
I wonder if it's related to the new commit just merged #7074
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1720339792
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -91,7 +95,7 @@ enum StreamAction {
// how much of data is actually written yet to underlying stream
private long offset;
// how much data has been ingested into the stream
- private volatile long writeOffset;
+ private long writeOffset;
Review Comment:
wouldn't this cause problem? writeOffset's value is used in hsync() without any lock or synchronization.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -174,7 +180,7 @@ public KeyOutputStream(Builder b) {
* @param version the set of blocks that are pre-allocated.
* @param openVersion the version corresponding to the pre-allocation.
*/
- public synchronized void addPreallocateBlocks(OmKeyLocationInfoGroup version, long openVersion) {
Review Comment:
Got it. BlockOutputStreamEntryPool.addPreallocateBlocks() is synchronized so no need to synchronize here.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntryPool.java:
##########
@@ -379,7 +380,7 @@ BlockOutputStreamEntry getCurrentStreamEntry() {
* @return the new current open stream to write to
Review Comment:
update javadoc for the new parameter. When should it be true/false?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestHSync.java:
##########
@@ -227,7 +235,7 @@ public static void init() throws Exception {
(OpenKeyCleanupService) cluster.getOzoneManager().getKeyManager().getOpenKeyCleanupService();
openKeyCleanupService.suspend();
- preFinalizationChecks();
+// preFinalizationChecks();
Review Comment:
[HDDS-11292](https://issues.apache.org/jira/browse/HDDS-11292) fixed this problem. Need to rebase 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.
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #7047:
URL: https://github.com/apache/ozone/pull/7047#issuecomment-2299435342
> M M IS: Inconsistent synchronization of org.apache.hadoop.hdds.scm.storage.BlockOutputStream.xceiverClientFactory; locked 60% of time Unsynchronized access at BlockOutputStream.java:[line 575]
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on PR #7047:
URL: https://github.com/apache/ozone/pull/7047#issuecomment-2294156483
I'll rebase when finishing the test on this branch.
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1723720439
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java:
##########
@@ -581,7 +581,10 @@ private CompletableFuture<Message> writeStateMachineData(
writeChunkFuture.thenApply(r -> {
if (r.getResult() != ContainerProtos.Result.SUCCESS
&& r.getResult() != ContainerProtos.Result.CONTAINER_NOT_OPEN
- && r.getResult() != ContainerProtos.Result.CLOSED_CONTAINER_IO) {
+ && r.getResult() != ContainerProtos.Result.CLOSED_CONTAINER_IO
+ // After concurrent flushes are allowed on the same key, chunk file inconsistencies can happen and
Review Comment:
is this being fixed by [HDDS-11328](https://issues.apache.org/jira/browse/HDDS-11328)?
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -804,7 +808,7 @@ void checkOpen() throws IOException {
if (isClosed()) {
throw new IOException("BlockOutputStream has been closed.");
} else if (getIoException() != null) {
- adjustBuffersOnException();
+// adjustBuffersOnException();
Review Comment:
nit: remove this line.
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1724158340
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java:
##########
@@ -581,7 +581,10 @@ private CompletableFuture<Message> writeStateMachineData(
writeChunkFuture.thenApply(r -> {
if (r.getResult() != ContainerProtos.Result.SUCCESS
&& r.getResult() != ContainerProtos.Result.CONTAINER_NOT_OPEN
- && r.getResult() != ContainerProtos.Result.CLOSED_CONTAINER_IO) {
+ && r.getResult() != ContainerProtos.Result.CLOSED_CONTAINER_IO
+ // After concurrent flushes are allowed on the same key, chunk file inconsistencies can happen and
+ // that should now
Review Comment:
It is, thanks for spotting it. Updated.
--
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-11239. Fix KeyOutputStream's exception handling when calling hsync concurrently [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #7047:
URL: https://github.com/apache/ozone/pull/7047#discussion_r1724676403
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestHSync.java:
##########
@@ -667,6 +675,99 @@ static void runTestHSync(FileSystem fs, Path file,
}, 500, 3000);
}
+
+ public static Stream<Arguments> concurrentExceptionHandling() {
+ return Stream.of(
+ Arguments.of(4, 1),
+ Arguments.of(4, 4),
+ Arguments.of(8, 4)
+ );
+ }
+
+ @ParameterizedTest
+ @MethodSource("concurrentExceptionHandling")
+ public void testConcurrentExceptionHandling(int syncerThreads, int errors) throws Exception {
+ final String rootPath = String.format("%s://%s/", OZONE_OFS_URI_SCHEME, CONF.get(OZONE_OM_ADDRESS_KEY));
+ CONF.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, rootPath);
+
+ ErrorInjectorImpl errorInjector = new ErrorInjectorImpl();
+ XceiverClientManager.enableErrorInjection(errorInjector);
+
+ final String dir = OZONE_ROOT + bucket.getVolumeName() + OZONE_URI_DELIMITER + bucket.getName();
+
+ try (FileSystem fs = FileSystem.get(CONF)) {
+ final Path file = new Path(dir, "exceptionhandling");
+ byte[] data = new byte[8];
+ ThreadLocalRandom.current().nextBytes(data);
+ int writes;
+ try (FSDataOutputStream out = fs.create(file, true)) {
+ writes = runConcurrentWriteHSyncWithException(file, out, data, syncerThreads, errors, errorInjector);
+ }
+ validateWrittenFile(file, fs, data, writes);
+ fs.delete(file, false);
Review Comment:
> seems to be failing consistently: TestHSync.testUncommittedBlocks:515 expected: but was:
>
> I wonder if it's related to the new commit just merged #7074
To address the failure in `TestHsync#testUncommittedBlocks`, ensure that the `deletedTable` is empty after `fs.delete(file, false)`;
Rebase to master and add a line after `fs.delete(file, false);` to wait for `KeyDeletingService` cleaning up `deletedTable`.
```
waitForEmptyDeletedTable();
```
--
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