You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "xichen01 (via GitHub)" <gi...@apache.org> on 2023/10/31 15:57:29 UTC
[PR] HDDS-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
xichen01 opened a new pull request, #5524:
URL: https://github.com/apache/ozone/pull/5524
## What changes were proposed in this pull request?
To fix the problem of abnormal dirty data overwriting normal data when S3G writes data. Make Ozone S3 puts atomic writes, all successes total or failures
The main implementation is to check whether the data of the key is complete or not when committing, if the data written by the key is different from the expected size, the key is considered incomplete, and the commit operation is not executed.
```java
@Override
public synchronized void close() throws IOException {
//...
try {
//...
if (requiresAtomicWrite) {
long expectedSize = blockOutputStreamEntryPool.getDataSize();
Preconditions.checkArgument(expectedSize == offset,
String.format("Expected: %d and actual %d write sizes do not match",
expectedSize, offset));
}
blockOutputStreamEntryPool.commitKey(offset);
} finally {
blockOutputStreamEntryPool.cleanup();
}
}
```
- The `requiresAtomicWrite` is only `enable` in S3G, it will be set in `EndpointBase#initialization` , just like the `s3Auth`
- The `blockOutputStreamEntryPool.getDataSize()` is the `dataSize` that was passed in when the S3G created the key (including the MPU key).
- The dataSize is the
- `Content-Length`: normal put key
- `x-amz-decoded-content-length`: Stream mode uploaded key.
- `x-amz-copy-source-range`: The length specified in the MPU copy Range (Ozone does not currently support Copy Ranges for non-MPU keys.).
PS:
refer to AWS put doc https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutObject.html
> Amazon S3 never adds partial objects; if you receive a success response, Amazon S3 added the entire object to the bucket.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9526
## How was this patch tested?
unit test, manually test.
manually test.
```bash
[root@VM-8-3-centos ~]$ aws configure set default.s3.multipart_threshold 1GB
[root@VM-8-3-centos ~]$ aws s3 --endpoint-url http://localhost:9878 cp ~/500M.img s3://bucket1/500M.img
^Ccancelled: ctrl-c received
[root@VM-8-3-centos /tmp]$ aws s3 ls s3://bucket1/ --endpoint-url http://localhost:9878
// A key that has not been completely written should not be visible.
```
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1384134226
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -555,6 +565,12 @@ public synchronized void close() throws IOException {
if (!isException) {
Preconditions.checkArgument(writeOffset == offset);
}
+ if (atomicKeyCreation) {
+ long expectedSize = blockOutputStreamEntryPool.getDataSize();
+ Preconditions.checkState(expectedSize == offset,
Review Comment:
Please file an improvement Jira to not raise exception but refactor the code in a later PR?
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1378482432
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyDataStreamOutput.java:
##########
@@ -81,6 +81,14 @@ enum StreamAction {
private long clientID;
+ /**
+ * Indicates if an atomic write is required. When set to true,
+ * the amount of data written must match the declared size during the commit.
+ * A mismatch will prevent the commit from succeeding.
+ * This is essential for operations like S3 put to ensure atomicity.
+ */
+ private boolean requiresAtomicWrite;
Review Comment:
I would suggest change the requiresAtomicWrite to atomicKeyCreation.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #5524:
URL: https://github.com/apache/ozone/pull/5524#issuecomment-1788652504
> @xichen01 , thanks for provide the fix so quickly. Is it possible to do more manual tests, such as multi upload, and range multi upload case?
OK, I will do more test for this Patch
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1378498181
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -555,6 +565,12 @@ public synchronized void close() throws IOException {
if (!isException) {
Preconditions.checkArgument(writeOffset == offset);
}
+ if (requiresAtomicWrite) {
+ long expectedSize = blockOutputStreamEntryPool.getDataSize();
+ Preconditions.checkArgument(expectedSize == offset,
Review Comment:
And please catch this exception in ObjectEndpoint#put finally when OutputStream#close is called.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #5524:
URL: https://github.com/apache/ozone/pull/5524#issuecomment-1788591214
@xichen01 , thanks for provide the fix so quickly. Is it possible to do more manual tests, such as multi upload, and range multi upload case?
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1378479679
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -882,6 +888,20 @@ private Response createMultipartKey(OzoneVolume volume, String bucket,
.createMultipartKey(ozoneBucket, key, length, partNumber,
uploadID, chunkSize, (DigestInputStream) body);
}
+
+ copyHeader = headers.getHeaderString(COPY_SOURCE_HEADER);
+ if (copyHeader != null) {
+ String range =
Review Comment:
Shall we add this COPY_SOURCE_HEADER_RANGE check in the put() L253 too?
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "guohao-rosicky (via GitHub)" <gi...@apache.org>.
guohao-rosicky commented on PR #5524:
URL: https://github.com/apache/ozone/pull/5524#issuecomment-1798452448
Thanks @xichen01 working on this,
I think it's a good change to just add `requiresAtomicWrite` to s3g and add an assertion on close, if there's an assertion error, it also goes into finally for cleanup and doesn't result in resource leakage.
Elegant code and detailed testing, LGTM.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1379348105
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -353,7 +357,14 @@ public Response put(
throw ex;
} finally {
if (output != null) {
- output.close();
+ try {
+ output.close();
+ } catch (IllegalStateException ex) {
+ LOG.error(String.format(
Review Comment:
This can occur due to client behavior and we should not be logging at level `error` a client with a bad connection could fill up the S3G logs.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1378488847
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -555,6 +565,12 @@ public synchronized void close() throws IOException {
if (!isException) {
Preconditions.checkArgument(writeOffset == offset);
}
+ if (requiresAtomicWrite) {
+ long expectedSize = blockOutputStreamEntryPool.getDataSize();
+ Preconditions.checkArgument(expectedSize == offset,
Review Comment:
Suggest to use Preconditions.checkState here, so it will throw out IllegalStateException instead of
IllegalArgumentException. IllegalStateException is more appropriate than IllegalArgumentException 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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1378539085
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -882,6 +888,20 @@ private Response createMultipartKey(OzoneVolume volume, String bucket,
.createMultipartKey(ozoneBucket, key, length, partNumber,
uploadID, chunkSize, (DigestInputStream) body);
}
+
+ copyHeader = headers.getHeaderString(COPY_SOURCE_HEADER);
+ if (copyHeader != null) {
+ String range =
Review Comment:
Ozone does not currently support copy ranges for non-MPU keys so Ozone will copy the entire key regardless of the value of `COPY_SOURCE_HEADER_RANGE`.
Maybe we can add this when we need to support copy ranges.
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPartUpload.java:
##########
@@ -135,4 +141,70 @@ public void testPartUploadWithIncorrectUploadID() throws Exception {
assertEquals(HTTP_NOT_FOUND, ex.getHttpCode());
}
}
+
+ @Test
+ public void testPartUploadStreamContentLength()
+ throws IOException, OS3Exception {
+ HttpHeaders headers = Mockito.mock(HttpHeaders.class);
+ ObjectEndpoint objectEndpoint = new ObjectEndpoint();
+ objectEndpoint.setHeaders(headers);
+ objectEndpoint.setClient(client);
+ objectEndpoint.setOzoneConfiguration(new OzoneConfiguration());
+ objectEndpoint.setHeaders(headers);
+ String keyName = UUID.randomUUID().toString();
+
+ String chunkedContent = "0a;chunk-signature=signature\r\n"
Review Comment:
The length of `"1234567890" + "abcde"` is 15, `05;chunk-signature=signature` is some metadata.
I referenced it here:
https://github.com/apache/ozone/blob/45aa6c1f1c21be86c63f134d94e50a156068603e/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestObjectPut.java#L168-L169
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1379569363
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -882,6 +888,20 @@ private Response createMultipartKey(OzoneVolume volume, String bucket,
.createMultipartKey(ozoneBucket, key, length, partNumber,
uploadID, chunkSize, (DigestInputStream) body);
}
+
+ copyHeader = headers.getHeaderString(COPY_SOURCE_HEADER);
+ if (copyHeader != null) {
+ String range =
Review Comment:
I checked the CopyObject document, it looks like range copy is not supported.
https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #5524:
URL: https://github.com/apache/ozone/pull/5524#issuecomment-1789696685
@SaketaChalamchala @tanvipenumudy can you please take a look?
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1378505901
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPartUpload.java:
##########
@@ -135,4 +141,70 @@ public void testPartUploadWithIncorrectUploadID() throws Exception {
assertEquals(HTTP_NOT_FOUND, ex.getHttpCode());
}
}
+
+ @Test
+ public void testPartUploadStreamContentLength()
+ throws IOException, OS3Exception {
+ HttpHeaders headers = Mockito.mock(HttpHeaders.class);
+ ObjectEndpoint objectEndpoint = new ObjectEndpoint();
+ objectEndpoint.setHeaders(headers);
+ objectEndpoint.setClient(client);
+ objectEndpoint.setOzoneConfiguration(new OzoneConfiguration());
+ objectEndpoint.setHeaders(headers);
Review Comment:
This line is duplicate with L150.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "SaketaChalamchala (via GitHub)" <gi...@apache.org>.
SaketaChalamchala commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1379512211
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -289,6 +291,8 @@ public Response put(
.equals(headers.getHeaderString("x-amz-content-sha256"))) {
body = new DigestInputStream(new SignedChunksInputStream(body),
E_TAG_PROVIDER.get());
+ length = Long.parseLong(
+ headers.getHeaderString(DECODED_CONTENT_LENGTH_HEADER));
Review Comment:
nit: Can reuse `amzDecodedLength` variable 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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1383512460
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -289,6 +291,8 @@ public Response put(
.equals(headers.getHeaderString("x-amz-content-sha256"))) {
body = new DigestInputStream(new SignedChunksInputStream(body),
E_TAG_PROVIDER.get());
+ length = Long.parseLong(
+ headers.getHeaderString(DECODED_CONTENT_LENGTH_HEADER));
Review Comment:
Done.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -882,17 +895,28 @@ private Response createMultipartKey(OzoneVolume volume, String bucket,
.createMultipartKey(ozoneBucket, key, length, partNumber,
uploadID, chunkSize, (DigestInputStream) body);
}
- ozoneOutputStream = getClientProtocol().createMultipartKey(
- volume.getName(), bucket, key, length, partNumber, uploadID);
if (copyHeader != null) {
Pair<String, String> result = parseSourceHeader(copyHeader);
-
String sourceBucket = result.getLeft();
String sourceKey = result.getRight();
OzoneKeyDetails sourceKeyDetails = getClientProtocol().getKeyDetails(
volume.getName(), sourceBucket, sourceKey);
+ String range =
+ headers.getHeaderString(COPY_SOURCE_HEADER_RANGE);
+ if (range != null) {
+ RangeHeader rangeHeader =
Review Comment:
Done.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -289,6 +291,8 @@ public Response put(
.equals(headers.getHeaderString("x-amz-content-sha256"))) {
body = new DigestInputStream(new SignedChunksInputStream(body),
E_TAG_PROVIDER.get());
+ length = Long.parseLong(
+ headers.getHeaderString(DECODED_CONTENT_LENGTH_HEADER));
Review Comment:
Done.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -882,17 +895,28 @@ private Response createMultipartKey(OzoneVolume volume, String bucket,
.createMultipartKey(ozoneBucket, key, length, partNumber,
uploadID, chunkSize, (DigestInputStream) body);
}
- ozoneOutputStream = getClientProtocol().createMultipartKey(
- volume.getName(), bucket, key, length, partNumber, uploadID);
if (copyHeader != null) {
Pair<String, String> result = parseSourceHeader(copyHeader);
-
String sourceBucket = result.getLeft();
String sourceKey = result.getRight();
OzoneKeyDetails sourceKeyDetails = getClientProtocol().getKeyDetails(
volume.getName(), sourceBucket, sourceKey);
+ String range =
+ headers.getHeaderString(COPY_SOURCE_HEADER_RANGE);
+ if (range != null) {
+ RangeHeader rangeHeader =
Review Comment:
Done.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1379570226
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPartUpload.java:
##########
@@ -135,4 +141,70 @@ public void testPartUploadWithIncorrectUploadID() throws Exception {
assertEquals(HTTP_NOT_FOUND, ex.getHttpCode());
}
}
+
+ @Test
+ public void testPartUploadStreamContentLength()
+ throws IOException, OS3Exception {
+ HttpHeaders headers = Mockito.mock(HttpHeaders.class);
+ ObjectEndpoint objectEndpoint = new ObjectEndpoint();
+ objectEndpoint.setHeaders(headers);
+ objectEndpoint.setClient(client);
+ objectEndpoint.setOzoneConfiguration(new OzoneConfiguration());
+ objectEndpoint.setHeaders(headers);
+ String keyName = UUID.randomUUID().toString();
+
+ String chunkedContent = "0a;chunk-signature=signature\r\n"
Review Comment:
Okay.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1379350076
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -353,7 +357,14 @@ public Response put(
throw ex;
} finally {
if (output != null) {
- output.close();
+ try {
+ output.close();
+ } catch (IllegalStateException ex) {
+ LOG.error(String.format(
Review Comment:
The metric at line 320 should be moved below into the `finally` block.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #5524:
URL: https://github.com/apache/ozone/pull/5524#issuecomment-1803907390
Thanks @xichen01 for this contribution, @kerneltime @adoroszlai @SaketaChalamchala for the code review.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5524:
URL: https://github.com/apache/ozone/pull/5524#issuecomment-1800221591
@ChenSammi @kerneltime @SaketaChalamchala please take a look at the latest patch
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1378562201
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -555,6 +565,12 @@ public synchronized void close() throws IOException {
if (!isException) {
Preconditions.checkArgument(writeOffset == offset);
}
+ if (requiresAtomicWrite) {
+ long expectedSize = blockOutputStreamEntryPool.getDataSize();
+ Preconditions.checkArgument(expectedSize == offset,
Review Comment:
Done.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyDataStreamOutput.java:
##########
@@ -81,6 +81,14 @@ enum StreamAction {
private long clientID;
+ /**
+ * Indicates if an atomic write is required. When set to true,
+ * the amount of data written must match the declared size during the commit.
+ * A mismatch will prevent the commit from succeeding.
+ * This is essential for operations like S3 put to ensure atomicity.
+ */
+ private boolean requiresAtomicWrite;
Review Comment:
Done
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1378565248
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPartUpload.java:
##########
@@ -135,4 +141,70 @@ public void testPartUploadWithIncorrectUploadID() throws Exception {
assertEquals(HTTP_NOT_FOUND, ex.getHttpCode());
}
}
+
+ @Test
+ public void testPartUploadStreamContentLength()
+ throws IOException, OS3Exception {
+ HttpHeaders headers = Mockito.mock(HttpHeaders.class);
+ ObjectEndpoint objectEndpoint = new ObjectEndpoint();
+ objectEndpoint.setHeaders(headers);
+ objectEndpoint.setClient(client);
+ objectEndpoint.setOzoneConfiguration(new OzoneConfiguration());
+ objectEndpoint.setHeaders(headers);
Review Comment:
Done.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #5524:
URL: https://github.com/apache/ozone/pull/5524#issuecomment-1789696057
There are few test failures in your fork, can you please take a look https://github.com/xichen01/ozone/actions/runs/6717923197/job/18257546921
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #5524:
URL: https://github.com/apache/ozone/pull/5524#issuecomment-1796975842
Can you look into the test failure in your branch https://github.com/xichen01/ozone/actions/runs/6773687585/job/18410561865
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1383553020
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -555,6 +565,12 @@ public synchronized void close() throws IOException {
if (!isException) {
Preconditions.checkArgument(writeOffset == offset);
}
+ if (atomicKeyCreation) {
+ long expectedSize = blockOutputStreamEntryPool.getDataSize();
+ Preconditions.checkState(expectedSize == offset,
Review Comment:
Understood, but throwing an exception is currently the most efficient and simple solution. If we don't throw exceptions, we may need to do a lot of extra checking, which would require a lot of code changes.
One possible reason is the S3G uses file semantics rather than key semantics to write keys.
File semantics for writing a key requires 1. create, 2 write, 3 close (including commit), and it is difficult to pass some state between these three separate and distinct steps, whereas with key semantics for put (put=create+write+close), where all the state is managed inside the put, it is simpler.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1384442898
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -555,6 +565,12 @@ public synchronized void close() throws IOException {
if (!isException) {
Preconditions.checkArgument(writeOffset == offset);
}
+ if (atomicKeyCreation) {
+ long expectedSize = blockOutputStreamEntryPool.getDataSize();
+ Preconditions.checkState(expectedSize == offset,
Review Comment:
OK, https://issues.apache.org/jira/browse/HDDS-9640
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1378507822
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPartUpload.java:
##########
@@ -135,4 +141,70 @@ public void testPartUploadWithIncorrectUploadID() throws Exception {
assertEquals(HTTP_NOT_FOUND, ex.getHttpCode());
}
}
+
+ @Test
+ public void testPartUploadStreamContentLength()
+ throws IOException, OS3Exception {
+ HttpHeaders headers = Mockito.mock(HttpHeaders.class);
+ ObjectEndpoint objectEndpoint = new ObjectEndpoint();
+ objectEndpoint.setHeaders(headers);
+ objectEndpoint.setClient(client);
+ objectEndpoint.setOzoneConfiguration(new OzoneConfiguration());
+ objectEndpoint.setHeaders(headers);
+ String keyName = UUID.randomUUID().toString();
+
+ String chunkedContent = "0a;chunk-signature=signature\r\n"
Review Comment:
@xichen01 , could you explain a little bit about this chunkedContent? Don't understand why this string length is 15.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1379351572
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -555,6 +565,12 @@ public synchronized void close() throws IOException {
if (!isException) {
Preconditions.checkArgument(writeOffset == offset);
}
+ if (atomicKeyCreation) {
+ long expectedSize = blockOutputStreamEntryPool.getDataSize();
+ Preconditions.checkState(expectedSize == offset,
Review Comment:
Since expect this failure mode to occur on a regular basis due to client connection issues, we should not be using pre condition but a simple check and skipping commit key if the entire key has not been written at close.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "xichen01 (via GitHub)" <gi...@apache.org>.
xichen01 commented on PR #5524:
URL: https://github.com/apache/ozone/pull/5524#issuecomment-1797688406
> Can you look into the test failure in your branch https://github.com/xichen01/ozone/actions/runs/6773687585/job/18410561865
The failed test are:
```bash
Prepare Ozone Manager | FAIL |
Test timeout 5 minutes exceeded.
--
Prepare :: Prepares OMs | FAIL |
1 test, 0 passed, 1 failed
```
This may have been an unstable test, I didn't change the OM code, when I reran the test, the test succeeded.
https://github.com/xichen01/ozone/actions/runs/6773687585
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "SaketaChalamchala (via GitHub)" <gi...@apache.org>.
SaketaChalamchala commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1379521623
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -882,17 +895,28 @@ private Response createMultipartKey(OzoneVolume volume, String bucket,
.createMultipartKey(ozoneBucket, key, length, partNumber,
uploadID, chunkSize, (DigestInputStream) body);
}
- ozoneOutputStream = getClientProtocol().createMultipartKey(
- volume.getName(), bucket, key, length, partNumber, uploadID);
if (copyHeader != null) {
Pair<String, String> result = parseSourceHeader(copyHeader);
-
String sourceBucket = result.getLeft();
String sourceKey = result.getRight();
OzoneKeyDetails sourceKeyDetails = getClientProtocol().getKeyDetails(
volume.getName(), sourceBucket, sourceKey);
+ String range =
+ headers.getHeaderString(COPY_SOURCE_HEADER_RANGE);
+ if (range != null) {
+ RangeHeader rangeHeader =
Review Comment:
nit: Can move `rangeHeader` declaration out of the if block to L908 and reuse `rangeHeader` in the block at L933.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1379612083
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -555,6 +565,12 @@ public synchronized void close() throws IOException {
if (!isException) {
Preconditions.checkArgument(writeOffset == offset);
}
+ if (atomicKeyCreation) {
+ long expectedSize = blockOutputStreamEntryPool.getDataSize();
+ Preconditions.checkState(expectedSize == offset,
Review Comment:
If an error code needs to be returned to the client, we should explicitly check that before closing. Throwing an exception is too heavy-handed for a regular check.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on code in PR #5524:
URL: https://github.com/apache/ozone/pull/5524#discussion_r1379351572
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -555,6 +565,12 @@ public synchronized void close() throws IOException {
if (!isException) {
Preconditions.checkArgument(writeOffset == offset);
}
+ if (atomicKeyCreation) {
+ long expectedSize = blockOutputStreamEntryPool.getDataSize();
+ Preconditions.checkState(expectedSize == offset,
Review Comment:
Since expect this failure mode to occur on a regular basis due to client connection issues, we should not be using pre condition but a simple check and skipping commit key if the entire key has not been written at close. We can add metrics to track how many over and under objects were uploaded.
--
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-9526. Two S3G instances writing the same key may cause data loss in case of an exception. [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi merged PR #5524:
URL: https://github.com/apache/ozone/pull/5524
--
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