kbendick commented on a change in pull request #2641:
URL: https://github.com/apache/iceberg/pull/2641#discussion_r640339570
##########
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))
Review comment:
Is this log also missing a placeholder? Seems like it has two arguments,
but only one placeholder.
My IDE doesn't seem to complain, but it looks like the same situation you're
fixing. Possibly it's not complaining because it's in a lambda? If you could
look into that it would be great 👍.
##########
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 {}",
jobContext.getJobID(), output);`? 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 MR 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]
