Skip to content

bug fix for lookup leak when we remove the last lookup from lookup tier#8598

Merged
himanshug merged 4 commits intoapache:masterfrom
FaxianZhao:bug_fix_for_lookup_tier
Sep 27, 2019
Merged

bug fix for lookup leak when we remove the last lookup from lookup tier#8598
himanshug merged 4 commits intoapache:masterfrom
FaxianZhao:bug_fix_for_lookup_tier

Conversation

@FaxianZhao
Copy link
Copy Markdown
Contributor

Fixes bug which caused by #7852.

Description

Fixed the bug ...

Due to #7852, coordinator won't tell other node to dispose lookup when we remove the last lookup from a tier.

Renamed the class ...

Added a forbidden-apis entry ...


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.
  • added integration tests.
  • been tested in a test Druid cluster.

Key changed/added classes in this PR
  • LookupCoordinatorManager

@himanshug
Copy link
Copy Markdown
Contributor

@FaxianZhao thanks for discovering and finding the root cause.

try {
List<ListenableFuture<Map.Entry>> futures = new ArrayList<>();
for (Map.Entry<String, Map<String, LookupExtractorFactoryMapContainer>> tierEntry : allLookupTiers.entrySet()) {
for (String tier : lookupNodeDiscovery.getAllTiers()) {
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.

can you merge https://github.com/FaxianZhao/incubator-druid/pull/1 to add another block to note lookups that will never be loaded?

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.

can you merge FaxianZhao#1 to add another block to note lookups that will never be loaded?

Good idea.
But for the users who enable 'druid.lookup.lookupTierIsDatasource', will this warning confuse them?

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.

a quick look at the usage of that property tells me that it is not used by coordinator and shouldn't interfere.

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.

a quick look at the usage of that property tells me that it is not used by coordinator and shouldn't interfere.

Agree.

FaxianZhao and others added 2 commits September 27, 2019 09:52
warnings about lookups that will never be loaded
@himanshug himanshug merged commit e1b4a3a into apache:master Sep 27, 2019
Fokko pushed a commit to Fokko/druid that referenced this pull request Sep 30, 2019
…er (apache#8598)

* bug fix for lookup leak when we remove the last lookup from lookup tier

* warnings about lookups that will never be loaded

* fix unit test
jon-wei pushed a commit to implydata/druid-public that referenced this pull request Oct 9, 2019
…er (apache#8598)

* bug fix for lookup leak when we remove the last lookup from lookup tier

* warnings about lookups that will never be loaded

* fix unit test
@jon-wei jon-wei added this to the 0.17.0 milestone Dec 17, 2019
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