- Notifications
You must be signed in to change notification settings - Fork5.5k
Logged notebook type#5425
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Logged notebook type#5425
Uh oh!
There was an error while loading.Please reload this page.
Conversation
kevin-bates 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.
Great idea@pinarkavak - thank you. I just had some minor comments.
| self._kernel_connections[kernel_id]=0 | ||
| self.start_watching_activity(kernel_id) | ||
| self.log.info("Kernel started: %s"%kernel_id) | ||
| self.log.info("Kernel type: %s"%self._kernels[kernel_id].kernel_name) |
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.
Rather than introduce another INFO entry that might result in ambiguities when starting kernels simultaneously, would you be okay with extendingKernel started: <id> with a comma-separated "type" clause:Kernel started: <id>, type: <type>?
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.
Hmm - wondering ifname: might be more indicative and closer to what we use in the 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.
Thanks Kevin, great points, my first intention was not to change any existing log, but updated the code to address your comments.
| self._kernel_connections[kernel_id]=0 | ||
| self.start_watching_activity(kernel_id) | ||
| self.log.info("Kernel started: %s"%kernel_id) | ||
| self.log.info("Kernel started %s, name: %s"%(kernel_id,self._kernels[kernel_id].kernel_name)) |
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.
Thank you for the updates Pinar. I hate to nit, but I think the: followingstarted should probably be retained. The only reason is in case thereare "log scrappers", they'll probably be looking forKernel started: - which is along the lines of your thoughts about preserving log entries. I suspect any applications that perform log analysis and need the kernel start time would be targeting "Kernel started:" as their token and not so much the content to EOL.
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.
Thanks Kevin! I totally agree, I somehow removed it without noticing while updating the log, added it back.
kevin-bates 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.
LGTM - thanks Pinar!
No description provided.