[GitHub] [hadoop] steveloughran commented on a diff in pull request #5621: HADOOP-18688. S3A audit info to include num of items in delete ops

["steveloughran (via GitHub)" <gi...@apache.org>]

steveloughran commented on code in PR #5621:
URL: https://github.com/apache/hadoop/pull/5621#discussion_r1186901863


##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/performance/ITestS3ADeleteCost.java:
##########
@@ -181,6 +184,8 @@ public void testDeleteFileInDir() throws Throwable {
 
   @Test
   public void testDirMarkersSubdir() throws Throwable {
+    LogCapturer logCapturer =

Review Comment:
   what are you trying to do here? I really don't like log capture for its brittleness and ugliness.
   
   Add something in TestHttpReferrerAuditHeader to actually examine the header.



##########
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/auditing.md:
##########
@@ -221,22 +222,23 @@ https://audit.example.org/hadoop/1/op_rename/3c0d9b7e-2a63-43d9-a220-3c574d768ef
 Here are the fields which may be found in a request.
 If any of the field values were `null`, the field is omitted.
 
-| Name | Meaning | Example |
-|------|---------|---------|
-| `cm` | Command | `S3GuardTool$BucketInfo` |
-| `fs` | FileSystem ID | `af5943a9-b6f6-4eec-9c58-008982fc492a` |
-| `id` | Span ID | `3c0d9b7e-2a63-43d9-a220-3c574d768ef3-3` |
-| `ji` | Job ID  (S3A committer)| `(Generated by query engine)` |
-| `op` | Filesystem API call | `op_rename` |
-| `p1` | Path 1 of operation | `s3a://alice-london/path1` |
-| `p2` | Path 2 of operation | `s3a://alice-london/path2` |
-| `pr` | Principal | `alice` |
-| `ps` | Unique process UUID | `235865a0-d399-4696-9978-64568db1b51c` |
-| `rg` | GET request range | `100-200` |
-| `ta` | Task Attempt ID (S3A committer) | |
-| `t0` | Thread 0: thread span was created in | `100` |
-| `t1` | Thread 1: thread this operation was executed in | `200` |
-| `ts` | Timestamp (UTC epoch millis) | `1617116985923` |
+| Name | Meaning                                                                                              | Example |

Review Comment:
   don't do the ide reformat as it changes the width of the table every time. just add the new row



##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/performance/ITestS3ARenameCost.java:
##########
@@ -124,6 +126,22 @@ public void testRenameFileToDifferentDirectory() throws Throwable {
         withWhenDeleting(OBJECT_DELETE_OBJECTS,
             directoriesInPath + 1));
 
+    if (isKeepingMarkers()) {
+      String output = logCapturer.getOutput();
+      String[] logs = output.split("\\n");
+      assertTrue("Num of files to delete should be 1",

Review Comment:
   consider my reviews to be an automatic -1 on any assert where assertJ does better, especially things like their .contains() predicate. save time by using assertJ from the outset.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org