-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix #4254: adding debug logging for exec stream messages #4288
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,10 +164,11 @@ public void addQueryParameters(URLBuilder httpUrlBuilder) { | |
if (in != null || redirectingIn) { | ||
httpUrlBuilder.addQueryParameter("stdin", "true"); | ||
} | ||
if (output != null) { | ||
boolean debug = ExecWebSocketListener.LOGGER.isDebugEnabled(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This reveals an odd coupling between ExecWebSocketListener and this class. The name of the method suggests a wider usage of these parameters, however, they are only ultimately consumed at the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's partly an issue with the existing code structure. The entry class is PodOperationsImpl, but it's in a different package from the rest of the logic - presumably because there could eventually be a v2. That's how most of dsl.internal is used.
It's usage should be clear from the context :) You could also make the case that the classes in dsl.internal should move into core.v1 and have methods be made package private. |
||
if (output != null || debug) { | ||
httpUrlBuilder.addQueryParameter("stdout", "true"); | ||
} | ||
if (error != null || terminateOnError) { | ||
if (error != null || terminateOnError || debug) { | ||
httpUrlBuilder.addQueryParameter("stderr", "true"); | ||
} | ||
} | ||
|
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.
Could we keep this private and retrieve the logger in PodOperationContext using the same approach?
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.
Also, are we sure that this is the class/package/point where we want to add the flag to decide if these messages should be logged? Can we document how to enable this logging? Would it make sense to check for debug enabled at the base package?
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.
You mean enable this off of two separate loggers? I didn't like that approach, even if they share a common base.
Yes, they should be logged in the ExecWebSocketListener, and we have to make an earlier decision to include the query flags.
Yes, we just have to agree on what it should be first.
You could, it's just a bit more code to create / access a couple of additional loggers - call me lazy, but I'd rather reuse something that exists if possible.
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.
I meant doing this from
PodOperationContext
:It's still coupled, but at least we don't access the other class variable that should be used for internal purposes only.
However, I'm going over both #4288 (comment) and #4288 (comment) and as I see it's just that the structure is flawed.
The
PodOperationContext
is not only used for Exec operations, but for other operations too.For me it's not clear that a public
addQueryParameters
method is used only to add exec-related parameters. Maybe this method just doesn't belong here.I guess that at this point it doesn't matter anyway.