adoroszlai commented on code in PR #5644:
URL: https://github.com/apache/ozone/pull/5644#discussion_r1403573151
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -217,17 +219,19 @@ public Response put(
@QueryParam("uploadId") @DefaultValue("") String uploadID,
InputStream body) throws IOException, OS3Exception {
long startNanos = Time.monotonicNowNanos();
- S3GAction s3GAction = S3GAction.CREATE_KEY;
-
+ // Set to an Array so that S3GAction can be changed within the function.
+ final S3GAction[] s3GAction = new S3GAction[] {S3GAction.CREATE_KEY};
Review Comment:
I really would like to avoid changing `action` (and sometimes even `success`
flag) to a modifiable reference so that it can be modified by methods being
called from this one. The current mixed approach of performing parts of the
operation in this method, parts in sub-methods, is already error-prone, and
this makes it worse.
I would prefer a clear `switch`-like decision of what operation is being
requested, then calling a specific method for that. It would also help avoid
the need for suppressing warnings about method length.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/audit/S3GAction.java:
##########
@@ -36,8 +36,12 @@ public enum S3GAction implements AuditAction {
//ObjectEndpoint
CREATE_MULTIPART_KEY,
+ CREATE_MULTIPART_KEY_STREAMING,
Review Comment:
I think it would be better to indicate the implementation as an entry in the
perf. map (e.g. `implementation: streaming"), instead of duplicating all
related operation types.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -113,6 +116,8 @@ public Response get(
@Context HttpHeaders hh) throws OS3Exception, IOException {
long startNanos = Time.monotonicNowNanos();
S3GAction s3GAction = S3GAction.GET_BUCKET;
+ Map<String, String> perf = new HashMap<>();
Review Comment:
Please use `TreeMap` for predictable order.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/audit/AuditMessage.java:
##########
@@ -109,15 +113,23 @@ public Builder withException(Throwable ex) {
return this;
}
+ public Builder setPerformance(Map<String, String> perf) {
+ this.performance = perf;
+ return this;
+ }
+
public AuditMessage build() {
- return new AuditMessage(user, ip, op, params, ret, throwable);
+ return new AuditMessage(user, ip, op, params, ret, throwable,
+ performance);
}
}
private String formMessage(String userStr, String ipStr, String opStr,
- Map<String, String> paramsMap, String retStr) {
+ Map<String, String> paramsMap, String retStr,
+ Map<String, String> performanceMap) {
return "user=" + userStr + " | ip=" + ipStr + " | " + "op=" + opStr
- + " " + paramsMap + " | " + "ret=" + retStr;
+ + " " + paramsMap + " performance=" + performanceMap
+ + " | ret=" + retStr;
Review Comment:
New content should be logged at the end of the message, and it should be
optional (avoid `performance=null`).
Something like:
```java
String perf = performanceMap != null && !performanceMap.isEmpty()
? " | perf=" + performanceMap
: "";
return "user=" + userStr + " | ip=" + ipStr + " | " + "op=" + opStr
+ " " + paramsMap + " | ret=" + retStr + perf;
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/audit/AuditMessage.java:
##########
@@ -109,15 +113,23 @@ public Builder withException(Throwable ex) {
return this;
}
+ public Builder setPerformance(Map<String, String> perf) {
+ this.performance = perf;
+ return this;
+ }
+
public AuditMessage build() {
- return new AuditMessage(user, ip, op, params, ret, throwable);
+ return new AuditMessage(user, ip, op, params, ret, throwable,
+ performance);
}
}
private String formMessage(String userStr, String ipStr, String opStr,
- Map<String, String> paramsMap, String retStr) {
+ Map<String, String> paramsMap, String retStr,
+ Map<String, String> performanceMap) {
Review Comment:
`performanceMap` can be defined as `Map<String, Object>` do avoid eager
`toString` conversion. (The same applies to `paramsMap`, but unfortunately
that one is already widely used, so we don't have to change it now. But we can
make it right from the start with the new map.)
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java:
##########
@@ -264,12 +269,16 @@ public Response get(
response.setTruncated(false);
}
- AUDIT.logReadSuccess(buildAuditMessageForSuccess(s3GAction,
- getAuditParameters()));
int keyCount =
response.getCommonPrefixes().size() + response.getContents().size();
- getMetrics().updateGetBucketSuccessStats(startNanos);
+ long getBucketLatency =
+ getMetrics().updateGetBucketSuccessStats(startNanos);
getMetrics().incListKeyCount(keyCount);
+ perf.put("getBucketKeyCount", String.valueOf(keyCount));
+ perf.put("getBucketLatencyMs",
Review Comment:
Log will show `op`, so we can omit `getBucket`. Also applies to all other
performance map keys (`createKeyLatencyMs`, etc.).
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -297,33 +304,35 @@ public Response put(
long putLength;
String eTag = null;
if (datastreamEnabled && !enableEC && length > datastreamMinLength) {
- getMetrics().updatePutKeyMetadataStats(startNanos);
+ s3GAction[0] = S3GAction.CREATE_KEY_STREAMING;
Pair<String, Long> keyWriteResult = ObjectEndpointStreaming
.put(bucket, keyPath, length, replicationConfig, chunkSize,
- customMetadata, (DigestInputStream) body);
+ customMetadata, (DigestInputStream) body, perf);
eTag = keyWriteResult.getKey();
putLength = keyWriteResult.getValue();
} else {
try (OzoneOutputStream output = getClientProtocol().createKey(
volume.getName(), bucketName, keyPath, length, replicationConfig,
customMetadata)) {
- getMetrics().updatePutKeyMetadataStats(startNanos);
+ long metadataLatency =
+ getMetrics().updatePutKeyMetadataStats(startNanos);
+ perf.put("metadataLatencyMs", nanosToMillisString(metadataLatency));
putLength = IOUtils.copyLarge(body, output);
eTag = DatatypeConverter.printHexBinary(
((DigestInputStream) body).getMessageDigest().digest())
.toLowerCase();
output.getMetadata().put(ETAG, eTag);
}
}
-
getMetrics().incPutKeySuccessLength(putLength);
+ perf.put("putLength", String.valueOf(putLength));
Review Comment:
I suggest using `"size"` for all read/write operations. Operation name
already shown, provides context.
Also, please create constants for common keys.
--
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]
