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

Implement default methods in TaskListener and BuildListener #3122

Merged
merged 1 commit into from Nov 19, 2017

Conversation

6 participants
@jglick
Member

jglick commented Oct 31, 2017

Would avoid the need for LessAbstractTaskListener in jenkinsci/workflow-support-plugin#15, for example.

@reviewbybees

@jglick jglick requested a review from svanoort Oct 31, 2017

@jglick jglick referenced this pull request Oct 31, 2017

Merged

[JEP-210] Log handling rewrite #15

5 of 6 tasks complete
@reviewbybees

This comment has been minimized.

reviewbybees commented Oct 31, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@@ -82,12 +70,13 @@ public void close() {
private final StackTraceElement caller;
private final ByteArrayOutputStream baos = new ByteArrayOutputStream();
public LogOutputStream(Logger logger, Level level, StackTraceElement caller) {
LogOutputStream(Logger logger, Level level, StackTraceElement caller) {

This comment has been minimized.

@svanoort

svanoort Nov 1, 2017

Member

🐛 - Potential breaker for code depending on this constructor.

This comment has been minimized.

@jglick

jglick Nov 2, 2017

Member

Not true; the class was private already, so the public modifier on its constructor was just misleading.

@@ -58,7 +52,7 @@
*
* @author Kohsuke Kawaguchi
*/
public class StreamTaskListener extends AbstractTaskListener implements Serializable, Closeable {
public class StreamTaskListener implements TaskListener, Closeable {

This comment has been minimized.

@svanoort

svanoort Nov 1, 2017

Member

🐛 You can no longer cast to AbstractTaskListener or check for instanceof AbstractTaskListener.

This comment has been minimized.

@jglick

jglick Nov 2, 2017

Member

That is true, but AbstractTaskListener was only ever a convenience base class, so there was never a legitimate reason to do so to begin with.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 3, 2017

Member

I was unable to find the implementation, which may be affected. But I agree that keeping the inheritance from a deprecated class would be the safest option for now. This old class is not doing anything wrong anyway

This comment has been minimized.

@jglick

jglick Nov 6, 2017

Member

As in #2879 I would prefer to eliminate all usages of the deprecated type to reduce confusion, since there is no plausible risk from the technical API change here.

@svanoort

A couple small compatibility issues but otherwise looks reasonable AFAICT.

@jglick

Not issues I think.

@@ -82,12 +70,13 @@ public void close() {
private final StackTraceElement caller;
private final ByteArrayOutputStream baos = new ByteArrayOutputStream();
public LogOutputStream(Logger logger, Level level, StackTraceElement caller) {
LogOutputStream(Logger logger, Level level, StackTraceElement caller) {

This comment has been minimized.

@jglick

jglick Nov 2, 2017

Member

Not true; the class was private already, so the public modifier on its constructor was just misleading.

@@ -58,7 +52,7 @@
*
* @author Kohsuke Kawaguchi
*/
public class StreamTaskListener extends AbstractTaskListener implements Serializable, Closeable {
public class StreamTaskListener implements TaskListener, Closeable {

This comment has been minimized.

@jglick

jglick Nov 2, 2017

Member

That is true, but AbstractTaskListener was only ever a convenience base class, so there was never a legitimate reason to do so to begin with.

@jglick jglick added the needs-review label Nov 2, 2017

@jglick jglick requested a review from oleg-nenashev Nov 6, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Nov 6, 2017

No strong opinion since I do not see any usages which will be affected by the change.
Would be great to get more votes from @jenkinsci/code-reviewers in order to elaborate whether it's fine to break binary compatibility for such cleanup.

@jglick

This comment has been minimized.

Member

jglick commented Nov 13, 2017

I do not see any usages which will be affected by the change.

And I would be happy to break any such usage if there were one, because it would be ridiculous.

@jglick jglick requested a review from svanoort Nov 13, 2017

@oleg-nenashev

Weak 🐝 since I cannot find any usages of the API to be broken. @reviewbybees done, I'd guess

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Nov 19, 2017

formally it's ready to go. YOLO.

@oleg-nenashev oleg-nenashev merged commit 5b9ad64 into jenkinsci:master Nov 19, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

@jglick jglick deleted the jglick:TaskListener branch Nov 20, 2017

@jtnord

This comment has been minimized.

Member

jtnord commented Mar 9, 2018

noting that this caused a breakage as the class serialVersionUID are different in the AbstractTaskListener - so Serialized formats are no longer the compatable.
The follow up #3154 did not restore this :-(

@jglick

This comment has been minimized.

Member

jglick commented Mar 9, 2018

@jtnord Normally that would be irrelevant since master → agent connections will use identical bytecode. Would only matter for other uses of Remoting involving diachronic serialization. Probably fixable by adding a serialVersionUID to AbstractTaskListener matching the original value.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Mar 10, 2018

@jtnord Do you have an issue reference for that?

@jtnord

This comment has been minimized.

Member

jtnord commented Mar 12, 2018

jglick added a commit to jglick/workflow-support-plugin that referenced this pull request Jun 11, 2018

jglick added a commit to jglick/workflow-durable-task-step-plugin that referenced this pull request Jun 11, 2018

orgi pushed a commit to orgi/workflow-durable-task-step-plugin that referenced this pull request Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment