You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "jojochuang (via GitHub)" <gi...@apache.org> on 2023/01/26 03:22:23 UTC
[GitHub] [ozone] jojochuang opened a new pull request, #4211: HDDS-7688. Ozone Manager change to support HSync.
jojochuang opened a new pull request, #4211:
URL: https://github.com/apache/ozone/pull/4211
## What changes were proposed in this pull request?
1. Repurpose CommitKey request handler to support hsync.
2. Make sure client hsync() updates SCM.
3. add StreamCapability interface to output stream.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7688
## How was this patch tested?
Unit tests
--
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
[GitHub] [ozone] szetszwo commented on a diff in pull request #4211: HDDS-7688. Ozone Manager change to support HSync.
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4211:
URL: https://github.com/apache/ozone/pull/4211#discussion_r1088585959
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1232,6 +1232,7 @@ message ListKeysResponse {
message CommitKeyRequest {
required KeyArgs keyArgs = 1;
required uint64 clientID = 2;
+ optional bool finalUpdate = 3;
Review Comment:
We should set default for backward compatibility. Since the default of bool is `false` (and we cannot change it in proto3), let's revert the boolean and change it to `hsync`.
(Also, when `commitKeyRequest.hasFinalUpdate() == false` in a request, it actually means that `finalUpdate == true`. It is going to be confusing.)
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -553,7 +553,7 @@ private void writeChunk(ChunkBuffer buffer)
/**
* @param close whether the flush is happening as part of closing the stream
*/
- private void handleFlush(boolean close)
+ protected void handleFlush(boolean close)
Review Comment:
Let's rename the current `handleFlush` to `handleFlushInternal` and then add a new `handleFlush` with try-catch as below.
```java
@@ -553,7 +544,31 @@ private void writeChunk(ChunkBuffer buffer)
/**
* @param close whether the flush is happening as part of closing the stream
*/
- private void handleFlush(boolean close)
+ void handleFlush(boolean close) throws IOException{
+ try {
+ handleFlushInternal(close);
+ } catch (ExecutionException e) {
+ // just set the exception here as well in order to maintain sanctity of
+ // ioException field
+ handleExecutionException(e);
+ } catch (InterruptedException ex) {
+ Thread.currentThread().interrupt();
+ handleInterruptedException(ex, true);
+ } catch (Throwable e) {
+ String msg = "Failed to flush. error: " + e.getMessage();
+ LOG.error(msg, e);
+ throw e;
+ } finally {
+ if (close) {
+ cleanup(false);
+ }
+ }
+ }
+
+ private void handleFlushInternal(boolean close)
throws IOException, InterruptedException, ExecutionException {
checkOpen();
// flush the last chunk data residing on the currentBuffer
```
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntryPool.java:
##########
@@ -345,6 +345,27 @@ void commitKey(long offset) throws IOException {
}
}
+ void hsyncKey(long offset) throws IOException {
+ if (keyArgs != null) {
+ // in test, this could be null
+ long length = getKeyLength();
+ Preconditions.checkArgument(offset == length,
+ "Epected offset: " + offset + " expected len: " + length);
Review Comment:
Typo: `Epected` should be `Expected`.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/OzoneOutputStream.java:
##########
@@ -99,4 +101,16 @@ public OmMultipartCommitUploadPartInfo getCommitUploadPartInfo() {
public OutputStream getOutputStream() {
return outputStream;
}
+
+ @Override
+ public boolean hasCapability(String capability) {
+ // Both KeyOutputStream and CryptoOutputStream implements
+ // StreamCapabilities interface
+ if (outputStream instanceof KeyOutputStream ||
+ outputStream instanceof CryptoOutputStream) {
Review Comment:
Check `if (outputStream instanceof StreamCapabilities)` instead.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java:
##########
@@ -97,6 +96,11 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMClientResponse omClientResponse = null;
boolean bucketLockAcquired = false;
Result result;
+ boolean isHSync = commitKeyRequest.hasFinalUpdate() &&
+ commitKeyRequest.getFinalUpdate() == false;
+ if (!isHSync) {
Review Comment:
Add metrics for hsync as below (we may do it in a separated JIRA)
```java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMetrics.java
@@ -63,6 +63,7 @@ public class OMMetrics {
private @Metric MutableCounterLong numTrashKeyLists;
private @Metric MutableCounterLong numVolumeLists;
private @Metric MutableCounterLong numKeyCommits;
+ private @Metric MutableCounterLong numKeyHsyncs;
private @Metric MutableCounterLong numBlockAllocations;
private @Metric MutableCounterLong numGetServiceLists;
private @Metric MutableCounterLong numBucketS3Lists;
@@ -665,6 +666,11 @@ public void incNumKeyCommits() {
numKeyCommits.incr();
}
+ public void incNumKeyHsyncs() {
+ numKeyOps.incr();
+ numKeyHsyncs.incr();
+ }
+
public void incNumKeyCommitFails() {
numKeyCommitFails.incr();
}
@@ -943,6 +949,11 @@ public long getNumKeyCommits() {
return numKeyCommits.value();
}
+ @VisibleForTesting
+ public long getNumKeyHsyncs() {
+ return numKeyHsyncs.value();
+ }
+
@VisibleForTesting
public long getNumKeyCommitFails() {
return numKeyCommitFails.value();
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java
index e9625c11a..3a4ac011e 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java
@@ -98,7 +98,9 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
Result result;
boolean isHSync = commitKeyRequest.hasFinalUpdate() &&
commitKeyRequest.getFinalUpdate() == false;
- if (!isHSync) {
+ if (isHSync) {
+ omMetrics.incNumKeyHsyncs();
+ } else {
omMetrics.incNumKeyCommits();
}
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java:
##########
@@ -49,15 +49,19 @@ public class OMKeyCommitResponse extends OmKeyResponse {
private OmBucketInfo omBucketInfo;
private RepeatedOmKeyInfo keysToDelete;
+ private Boolean isHSync;
+
public OMKeyCommitResponse(@Nonnull OMResponse omResponse,
@Nonnull OmKeyInfo omKeyInfo, String ozoneKeyName, String openKeyName,
- @Nonnull OmBucketInfo omBucketInfo, RepeatedOmKeyInfo keysToDelete) {
+ @Nonnull OmBucketInfo omBucketInfo, RepeatedOmKeyInfo keysToDelete,
+ boolean isHSync) {
Review Comment:
Fix the indentation.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCommitResponse.java:
##########
@@ -49,15 +49,19 @@ public class OMKeyCommitResponse extends OmKeyResponse {
private OmBucketInfo omBucketInfo;
private RepeatedOmKeyInfo keysToDelete;
+ private Boolean isHSync;
Review Comment:
Should it be the lower case `boolean`? Otherwise, we need to handle `null`.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java:
##########
@@ -91,6 +105,8 @@ enum StreamAction {
private long clientID;
+ private OzoneManagerProtocol omClient;
+
Review Comment:
`omClient` is unused. We should remove it.
--
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
[GitHub] [ozone] jojochuang commented on pull request #4211: HDDS-7688. Ozone Manager change to support HSync.
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #4211:
URL: https://github.com/apache/ozone/pull/4211#issuecomment-1404523663
@szetszwo let me know how you think. I thought about creating a new request type instead of repurposing CommitKey. But the amount of boilerplate code required to create a new request type is tremendous. This PR is more of a proof of concept to demo that it is doable.
--
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
[GitHub] [ozone] szetszwo merged pull request #4211: HDDS-7688. Ozone Manager change to support HSync.
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #4211:
URL: https://github.com/apache/ozone/pull/4211
--
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
[GitHub] [ozone] jojochuang commented on pull request #4211: HDDS-7688. Ozone Manager change to support HSync.
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #4211:
URL: https://github.com/apache/ozone/pull/4211#issuecomment-1410648147
Split the PR into two: #4205 / HDDS-7826 deals with adding StreamCapabilities API. And this PR is composed of two commits: #4205 and the implementation of OM hsync API.
--
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
[GitHub] [ozone] jojochuang commented on a diff in pull request #4211: HDDS-7688. Ozone Manager change to support HSync.
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #4211:
URL: https://github.com/apache/ozone/pull/4211#discussion_r1091258528
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java:
##########
@@ -97,6 +96,11 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OMClientResponse omClientResponse = null;
boolean bucketLockAcquired = false;
Result result;
+ boolean isHSync = commitKeyRequest.hasFinalUpdate() &&
+ commitKeyRequest.getFinalUpdate() == false;
+ if (!isHSync) {
Review Comment:
Filed https://issues.apache.org/jira/browse/HDDS-7855 as a follow up task.
--
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
[GitHub] [ozone] szetszwo commented on a diff in pull request #4211: HDDS-7688. Ozone Manager change to support HSync.
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4211:
URL: https://github.com/apache/ozone/pull/4211#discussion_r1091341060
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -553,7 +540,26 @@ private void writeChunk(ChunkBuffer buffer)
/**
* @param close whether the flush is happening as part of closing the stream
*/
- protected void handleFlush(boolean close)
+ void handleFlush(boolean close) throws IOException {
+ try {
+ handleFlushInternal(true);
Review Comment:
We should pass `close` to `handleFlushInternal`.
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -770,7 +763,7 @@ void handleInterruptedException(Exception ex,
* @param ex
* @throws IOException
*/
- private void handleExecutionException(Exception ex) throws IOException {
+ protected void handleExecutionException(Exception ex) throws IOException {
Review Comment:
After added `handleFlushInternal`, we can keep this method private.
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java:
##########
@@ -553,7 +540,26 @@ private void writeChunk(ChunkBuffer buffer)
/**
* @param close whether the flush is happening as part of closing the stream
*/
- private void handleFlush(boolean close)
+ void handleFlush(boolean close) throws IOException {
+ try {
+ handleFlushInternal(true);
+ } catch (ExecutionException e) {
+ handleExecutionException(e);
+ } catch (InterruptedException ex) {
+ Thread.currentThread().interrupt();
+ handleInterruptedException(ex, true);
+ } catch (Throwable e) {
+ String msg = "Failed to flush. error: " + e.getMessage();
+ LOG.error(msg, e);
+ throw e;
+ } finally {
+ if (close) {
+ cleanup(false);
+ }
+ }
+ }
+
+ protected void handleFlushInternal(boolean close)
Review Comment:
`handleFlushInternal` can be private.
--
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
[GitHub] [ozone] jojochuang commented on pull request #4211: HDDS-7688. Ozone Manager change to support HSync.
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #4211:
URL: https://github.com/apache/ozone/pull/4211#issuecomment-1413841364
Rebased the 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
[GitHub] [ozone] kerneltime commented on pull request #4211: HDDS-7688. Ozone Manager change to support HSync.
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #4211:
URL: https://github.com/apache/ozone/pull/4211#issuecomment-1409002163
@aswinshakil 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