Skip to content

Move common configurations to TuningConfig#10478

Merged
clintropolis merged 1 commit intoapache:masterfrom
liran-funaro:pr-tuning-config
Dec 4, 2020
Merged

Move common configurations to TuningConfig#10478
clintropolis merged 1 commit intoapache:masterfrom
liran-funaro:pr-tuning-config

Conversation

@liran-funaro
Copy link
Copy Markdown
Contributor

@liran-funaro liran-funaro commented Oct 5, 2020

Description

The goal of this PR is to reduce code duplications and redundant code.
It does not affect Druid's UX. Only the internal code structure.

  • Move common methods that are used in HadoopTuningConfig and in AppenderatorConfig to TuningConfig
  • Rename rowFlushBoundary in HadoopTuningConfig to maxRowsInMemory to match TuningConfig API
    • This was already defined as @JsonProperty("maxRowsInMemory"), but the method name did not match TuningConfig API

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • 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.

Key changed/added classes in this PR
  • TuningConfig
  • AppenderatorConfig
  • HadoopTuningConfig

Copy link
Copy Markdown

@Eshcar Eshcar left a comment

Choose a reason for hiding this comment

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

LGTM
All changes seem to improve code quality
can you just add a sentence (in the jira) explaining this is a preamble to a later PR that will add the I2 extension point

@liran-funaro liran-funaro marked this pull request as draft October 8, 2020 08:07
@liran-funaro liran-funaro mentioned this pull request Oct 8, 2020
8 tasks
@liran-funaro liran-funaro marked this pull request as ready for review October 8, 2020 15:48
@liran-funaro liran-funaro changed the title Refactor AppenderatorConfig to inherit from TuningConfig Move common methods that used in HadoopTuningConfig and in AppenderatorConfig to TuningConfig Oct 27, 2020
@liran-funaro liran-funaro changed the title Move common methods that used in HadoopTuningConfig and in AppenderatorConfig to TuningConfig Move common methods from HadoopTuningConfig and AppenderatorConfig to TuningConfig Oct 27, 2020
* Move common methods that are used in HadoopTuningConfig and in AppenderatorConfig to TuningConfig
* Rename rowFlushBoundary in HadoopTuningConfig to maxRowsInMemory to match TuningConfig API
@liran-funaro liran-funaro changed the title Move common methods from HadoopTuningConfig and AppenderatorConfig to TuningConfig Move common configurations to TuningConfig Oct 27, 2020
@clintropolis
Copy link
Copy Markdown
Member

I don't seem to be able to retry the failing CI which looks like it was a flaky test.

@clintropolis clintropolis merged commit 52d46ce into apache:master Dec 4, 2020
@liran-funaro
Copy link
Copy Markdown
Contributor Author

Thanks, @clintropolis.
I appreciate your help merging this PR.
Please also check out the follow-up PRs: #10593 and/or #10001.

@jihoonson jihoonson added this to the 0.21.0 milestone Jan 4, 2021
JulianJaffePinterest pushed a commit to JulianJaffePinterest/druid that referenced this pull request Jan 22, 2021
* Move common methods that are used in HadoopTuningConfig and in AppenderatorConfig to TuningConfig
* Rename rowFlushBoundary in HadoopTuningConfig to maxRowsInMemory to match TuningConfig API
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.

4 participants