kbendick commented on a change in pull request #2641:
URL: https://github.com/apache/iceberg/pull/2641#discussion_r640336576
##########
File path:
mr/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergOutputCommitter.java
##########
@@ -232,7 +232,7 @@ public void abortJob(JobContext originalContext, int
status) throws IOException
.executeWith(tableExecutor)
.onFailure((output, exc) -> LOG.warn("Failed cleanup table {} on
abort job", output, exc))
.run(output -> {
- LOG.info("Cleaning job for table {}", jobContext.getJobID(),
output);
+ LOG.info("Cleaning job for table {} {}", jobContext.getJobID(),
output);
Review comment:
Actually, small nit:
Could you make this `LOG.info("Cleaning table {} with job id {}", output,
jobContext.getJobID());`? I personally think having both arguments one after
another will be somewhat unclear, and the emphasis should be on the table being
cleaned in my opinion.
The way it is now, for a table `foo` for a MapReduce job
`job_201112212038_0004`, the output would be `Cleaning job for table foo
job_201112212038_0004`. To me, that personally isn't super clear.
I think `Cleaning table foo with job id job_201112212038_0004` would be much
more clear to users, and would provide a simpler / potentially more intuitive
string to search for in the logs (aka looking through cabana for "Cleaning
table foo").
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
