-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Various logging improvements #2899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
tillrohrmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me @uce. I had only some minor comments. After addressing them +1 for merging.
| # change the log levels here. | ||
| log4j.logger.akka=INFO | ||
| log4j.logger.org.apache.kafka=INFO | ||
| log4j.logger.org.apache.hadoop=INFO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also work for the shaded hadoop dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, at runtime they have a different package. Therefore, we need to change this here, good catch I think ;) @rmetzger Can you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not relocating Hadoop, so this configuration line is correct.
As you can see from our logs (the first line usually):
org.apache.hadoop.util.NativeCodeLoader
We are relocating some Hadoop dependencies (like Guava), and rewrite the Hadoop code to call them, but we are not relocating the Hadoop code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I remember ;) This is indeed shady business. But good to know that we can keep it as is.
| long stateSize = 0L; | ||
| for (SubtaskState subtaskState : subtaskStates.values()) { | ||
| stateSize += subtaskState.getStateSize(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we use the getStateSize method here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
| LOG.debug("Adding " + possibleHadoopConfPath + "/core-site.xml to hadoop configuration"); | ||
| } | ||
| } else { | ||
| LOG.debug("File " + possibleHadoopConfPath + "/core-site.xml not found."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{} for variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
| LOG.debug("Adding " + possibleHadoopConfPath + "/hdfs-site.xml to hadoop configuration"); | ||
| } | ||
| } else { | ||
| LOG.debug("File " + possibleHadoopConfPath + "/hdfs-site.xml not found."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here with {}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
|
We should also set the received and handled message in |
|
Added |
I would like to include the following fixes for the 1.1.4 release. In general, I try to improve the debugging experience with this.
/cc @tillrohrmann