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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
