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

Remove support for unbounded number of polling threads #3258

Merged
merged 2 commits into from Jan 28, 2018

Conversation

4 participants
@daniel-beck
Member

daniel-beck commented Jan 23, 2018

The default is 10. Previously unbounded configs will be set to 10.


I always thought it was weird to not allow >100 except that 0 means infinite…

WDYT @jenkinsci/code-reviewers ?

Proposed changelog entries

  • RFE: Always limit number of SCM polling threads to between 10 and 100.

Submitter checklist

  • [n/a] JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs
Remove support for unbounded number of polling threads
The default is 10. Previously unbounded configs will be set to 10.
@jglick

jglick approved these changes Jan 23, 2018

@@ -313,7 +319,7 @@ public void setPollingThreadCount(int n) {
@Restricted(NoExternalUse.class)
public boolean isPollingThreadCountOptionVisible() {
if (getPollingThreadCount() != 0) {
if (getPollingThreadCount() != 10) {
// this is a user who already configured the option

This comment has been minimized.

@jglick

jglick Jan 23, 2018

Member

Why does this logic even exist again?

This comment has been minimized.

@daniel-beck

daniel-beck Jan 23, 2018

Member

WDYM? Not showing an advanced option unless it's likely to be needed?

This comment has been minimized.

@jglick

jglick Jan 23, 2018

Member

Yeah I am not sure why code like this needs to exist. The normal UI is an Advanced… button, and if the value has been edited from its default (which can be inferred automatically by normal databinding), an “edited” icon appears on it.

This comment has been minimized.

@daniel-beck

daniel-beck Jan 23, 2018

Member

There's no non-advanced UI though, so it would be just a section title and an Advanced button?

This comment has been minimized.

@jglick

jglick Jan 23, 2018

Member

Sure.

To be clear, I am not trying to block this, just noting a related UI simplification we could make.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jan 23, 2018

Form validation needs to be improved a bit here.

@oleg-nenashev

🐝 and @reviewbybees done

@daniel-beck Do you plan to write more tests or do you want it to be merged as is?

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Jan 26, 2018

@oleg-nenashev Tests would be nice to have, but not sure by when I have the time to do them. No strong opinion.

@jglick

jglick approved these changes Jan 26, 2018

@jglick jglick added ready-for-merge and removed needs-review labels Jan 26, 2018

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 26, 2018

I am fine with merging as is

@aheritier

+1000

@oleg-nenashev oleg-nenashev merged commit 73e2c9f into jenkinsci:master Jan 28, 2018

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment