Skip to content

Update default maxSegmentsInNodeLoadingQueue#11540

Merged
suneet-s merged 3 commits intoapache:masterfrom
suneet-s:maxSegmentsInNodeLoadingQueue
Aug 5, 2021
Merged

Update default maxSegmentsInNodeLoadingQueue#11540
suneet-s merged 3 commits intoapache:masterfrom
suneet-s:maxSegmentsInNodeLoadingQueue

Conversation

@suneet-s
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s commented Aug 3, 2021

Description

Update the default maxSegmentsInNodeLoadingQueue from 0 (unbounded) to 100.

An unbounded maxSegmentsInNodeLoadingQueue can cause cluster instability.
Since this is the default druid operators need to run into this instability
and then look through the docs to see that the recommended value for a large
cluster is 1000. This change makes it so the default will prevent clusters
from falling over as they grow over time.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

Update the default maxSegmentsInNodeLoadingQueue from 0 (unbounded) to 100.

An unbounded maxSegmentsInNodeLoadingQueue can cause cluster instability.
Since this is the default druid operators need to run into this instability
and then look through the docs to see that the recommended value for a large
cluster is 1000. This change makes it so the default will prevent clusters
from falling over as they grow over time.
@suneet-s
Copy link
Copy Markdown
Contributor Author

suneet-s commented Aug 3, 2021

Added Design Review and Release Notes since this changes the default of an existing field.

@cryptoe
Copy link
Copy Markdown
Contributor

cryptoe commented Aug 4, 2021

LGTM.
+1 for tagging it into release notes.

// updated to Jackson 2.9 it could be changed, see https://github.com/apache/druid/issues/7152
@JsonProperty("killPendingSegmentsSkipList") Object dataSourcesToNotKillStalePendingSegmentsIn,
@JsonProperty("maxSegmentsInNodeLoadingQueue") int maxSegmentsInNodeLoadingQueue,
@JsonProperty(value = "maxSegmentsInNodeLoadingQueue") @Nullable Integer maxSegmentsInNodeLoadingQueue,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Is the value = part required?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops will remove

replicateAfterLoadTimeout == null ? DEFAULT_REPLICATE_AFTER_LOAD_TIMEOUT : replicateAfterLoadTimeout,
maxNonPrimaryReplicantsToLoad == null ? DEFAULT_MAX_NON_PRIMARY_REPLICANTS_TO_LOAD : maxNonPrimaryReplicantsToLoad
maxNonPrimaryReplicantsToLoad == null
? DEFAULT_MAX_NON_PRIMARY_REPLICANTS_TO_LOAD
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation seems off.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so too, but I used intelliJ's auto-format and this is what it came up with 🤷

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

@kfaraz
Copy link
Copy Markdown
Contributor

kfaraz commented Aug 4, 2021

LGTM

@suneet-s suneet-s merged commit e423e99 into apache:master Aug 5, 2021
@suneet-s suneet-s deleted the maxSegmentsInNodeLoadingQueue branch August 5, 2021 18:26
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants