Skip to content
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

[JENKINS-52165] Disable watch mode by default for now #84

Merged
merged 2 commits into from Oct 24, 2018

Conversation

Projects
None yet
3 participants
@jglick
Copy link
Member

jglick commented Oct 24, 2018

Disabling #63 to buy some time until I can fix some acknowledged issues (mainly JENKINS-54133 and waiting for the maintainer on JENKINS-54081) and properly investigate some reported issues which are likely linked (JENKINS-54073 and JENKINS-53888). Complements jenkins-infra/update-center2#247. jenkinsci/workflow-basic-steps-plugin#74 is still needed promptly since that seems to be independent.

Note that this retains watch mode in tests, to continue to enforce coverage in this plugin and in any other pkugins which currently have a test dep on 2.22+ which might bump up their dep to 2.25.

@jglick jglick requested review from abayer , rsandell , dwnusbaum , joseblas and svanoort Oct 24, 2018

@dwnusbaum
Copy link
Member

dwnusbaum left a comment

Ok, given the various issues I think it makes sense to go ahead and disable it until we have time to investigate.

@@ -170,7 +171,7 @@ public FormValidation doCheckReturnStatus(@QueryParameter boolean returnStdout,
/** If set to false, disables {@link Execution#watching} mode. */
@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "public & mutable only for tests")
@Restricted(NoExternalUse.class)
public static boolean USE_WATCHING = !"false".equals(System.getProperty(DurableTaskStep.class.getName() + ".USE_WATCHING"));
public static boolean USE_WATCHING = Boolean.parseBoolean(System.getProperty(DurableTaskStep.class.getName() + ".USE_WATCHING", Main.isUnitTest ? "true" : /* JENKINS-52165 turn back on by default */ "false"));

This comment has been minimized.

@dwnusbaum

dwnusbaum Oct 24, 2018

Member

It seems undesirable to not actually test using the mode that we are shipping to users, but I also see why it would be good to test against watch mode since any issues in downstream plugins are likely to be specific to that mode, so I am somewhat split on this.

This comment has been minimized.

@jglick

jglick Oct 24, 2018

Author Member

It seems undesirable to not actually test using the mode that we are shipping to users

Indeed. On the other hand the non-watch mode has been tested for years, and I certainly hope this situation will be temporary.

This comment has been minimized.

@svanoort

svanoort Oct 24, 2018

Member

I'm okay with this part -- not perfect but it'll do. Just trying to check that the exit condition stuff is 100% the same internally.

@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Oct 24, 2018

I was curious to see what tests failed if I did not use watch mode for tests, and among the expected failures related to remote logging, the tests for my recent JENKINS-28822 fix started failing, because I only fixed it for watch mode. It would be ideal to modify that fix to work for non-watch mode as well, or we'd need to reopen the issue. To fix it, we'd need to reproduce the logic related to causeOfStoppage currently in DurableTaskStep$Execution#exited in the legacy block of DurableTaskStep$Execution#check.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Oct 24, 2018

my recent JENKINS-28822 fix

#75 FTR. Indeed, that fix needs to be extended to work in both modes. I will check it.

@jglick jglick requested a review from dwnusbaum Oct 24, 2018

@@ -541,7 +541,7 @@ private Object writeReplace() {
j.waitForCompletion(b);
// Would have succeeded before https://github.com/jenkinsci/workflow-durable-task-step-plugin/pull/75.
j.assertBuildStatus(Result.ABORTED, b);
j.assertLogContains("Timeout has been exceeded", b);
j.waitForMessage("Timeout has been exceeded", b); // TODO assertLogContains fails unless a sleep is introduced; possible race condition in waitForCompletion

This comment has been minimized.

@dwnusbaum

dwnusbaum Oct 24, 2018

Member

I saw this as well, although it passed sometimes in my local testing, so some kind of race condition seems likely.

Throwable originalCause = causeOfStoppage;
if ((returnStatus && originalCause == null) || exitCode == 0) {
getContext().onSuccess(returnStatus ? exitCode : returnStdout ? new String(output, StandardCharsets.UTF_8) : null);
getContext().onSuccess(returnStatus ? exitCode : returnStdout ? new String(output.produce(), StandardCharsets.UTF_8) : null);

This comment has been minimized.

@svanoort

svanoort Oct 24, 2018

Member

🐜 Nested ternary operators are super hard to read for most developers without line breaks between them

This comment has been minimized.

@jglick

jglick Oct 24, 2018

Author Member

Perhaps. Anyway that predates this PR.

@svanoort
Copy link
Member

svanoort left a comment

Looks okay to me, let's release. BUT, I think we need a clean reproduction and verify-that-it's-fixed before we can release workflow-job again.

@svanoort
Copy link
Member

svanoort left a comment

Looks okay to me, let's release. BUT, I think we need a clean reproduction and verify-that-it's-fixed before we can release workflow-job again.

@svanoort

This comment has been minimized.

Copy link
Member

svanoort commented Oct 24, 2018

Double approval could be because I approve extra hard... or because conference wifi sucks. You decide!

@dwnusbaum dwnusbaum merged commit d330bb8 into jenkinsci:master Oct 24, 2018

2 checks passed

continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@jglick jglick deleted the jglick:disable-JENKINS-52165 branch Oct 24, 2018

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

Merge pull request jenkinsci#84 from jglick/disable-JENKINS-52165
[JENKINS-52165] Disable watch mode by default for now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.