Conversation
|
Good catch! |
|
@rymurr would you mind reviewing this? |
| .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); |
There was a problem hiding this comment.
Good catch 👍.
I seem to remember there being some sort of analyzer annotation or something to ensure the correct number of arguments, but for the life of me I can't recall right now. Probably wouldn't be related to this case.
EDIT: You don't need to use it, but for your reference this is what I was thinking of. It and some other annotations in the same package provides compile time checking on format strings: https://errorprone.info/api/latest/com/google/errorprone/annotations/FormatMethod.html
There was a problem hiding this 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 you have it 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").
There was a problem hiding this comment.
Sounds good, i'll do it.
| .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); |
There was a problem hiding this 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 you have it 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").
| @@ -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)) | |||
There was a problem hiding this 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 👍.
There was a problem hiding this comment.
For warning, the second arguments of exc is an Exception, so it is correct.
rymurr
left a comment
rymurr
left a comment
There was a problem hiding this comment.
LGTM, thanks @StefanXiepj
5 participants
This pr improved some code style.