Skip to content

Comments

Cleanup changes pulled out of PR #12368#12672

Merged
kfaraz merged 1 commit intoapache:masterfrom
paul-rogers:220616-cleanup
Jun 23, 2022
Merged

Cleanup changes pulled out of PR #12368#12672
kfaraz merged 1 commit intoapache:masterfrom
paul-rogers:220616-cleanup

Conversation

@paul-rogers
Copy link
Contributor

@paul-rogers paul-rogers commented Jun 17, 2022

The new IT PR has struggled to get a clean build due to the complexity and flakiness of the Druid build system. It is ironic that the attempt to fix the ITs has been stymied by the fragility of the existing ITs. Out of desperation, the big PR is being broken up into smaller chunks. This one provides the myriad cleanup tasks done in that PR.

The cleanup is of two kinds:

  • Just general cleanup: fixing misspellings, etc.
  • Allowing the use of some of Druid's "JSON config" objects in tests.

The general cleanup is self-explanatory. The JSON configs are odd beasts: there is no way to create one via code; only using the JSON config mechanism that populates the instance dynamically from a provided set of properties. While that is great in production, it does make it very hard to create the config in tests. Since we do, in fact, want to test, this PR alters a few of these configs to provide a constructor or factory method.

Further, the config key used for properties tends to reside in the consumer of those properties. If we do want to create one in tests, using the property mechanism, we have to copy/paste that key. To make life a bit easier here, this PR moves the key to a constant inside the config object itself (but only for those configs used by the IT PR.)

The ITs want to create a Guice instance for "client" use, not for use in a Druid server. The existing ITs jump though some amazing hoops to do this. The new ones take a somewhat different path. To make this possible, a few of the formerly-private bits related to Guice initialization are now exposed for use by tests.


This PR has:

  • been self-reviewed.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • 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.
  • been tested in a test Druid cluster.

Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

LGTM!!

Copy link
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

Thanks for the refactoring, @paul-rogers !
The changes LGTM.

@kfaraz kfaraz merged commit ffcb996 into apache:master Jun 23, 2022
paul-rogers added a commit to paul-rogers/druid that referenced this pull request Jun 24, 2022
@paul-rogers paul-rogers mentioned this pull request Jun 24, 2022
2 tasks
abhishekagarwal87 pushed a commit that referenced this pull request Jun 25, 2022
* Revert changes from #12672

* Reverted more conflicting changes

Changes are not needed given previous reversions.
@abhishekagarwal87 abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022
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