Skip to content

Fix removeDataSource() and removeSegment() of SQLMetadataSegmentManager - #5001

Merged
gianm merged 1 commit into
apache:masterfrom
jihoonson:fix-segment-manager
Oct 31, 2017
Merged

gianm merged 1 commit into
apache:masterfrom
jihoonson:fix-segment-manager

Conversation

@jihoonson

@jihoonson jihoonson commented Oct 24, 2017 •

Copy link
Copy Markdown
Contributor

removeDataSource() currently checks a snapshot of dataSources and returns false if the target dataSource doesn't exist in the snapshot. However, SQLMetadataSegmentManager updates the snapshot by polling periodically, and thus it's possible that the target dataSource isn't in the snapshot but in the metadata store. removeSegment() has a similar issue.
This patch fixes this issue. An unnecessary lock is also removed.


This change is Reviewable

@jihoonson jihoonson added the Bug label Oct 24, 2017

@nishantmonu51 nishantmonu51 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@gianm gianm left a comment

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.

LGTM.

@gianm
gianm merged commit e96daa2 into apache:master Oct 31, 2017
@jon-wei jon-wei added this to the 0.12.0 milestone Jan 5, 2018
final ConcurrentHashMap<String, DruidDataSource> emptyMap = new ConcurrentHashMap<>();
ConcurrentHashMap<String, DruidDataSource> current;
do {
current = dataSourcesRef.get();

@licl2014 licl2014 Apr 23, 2020 •

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.

Here, there is no guarantee that dataSourcesRef is safe. it may be updated in for loop above. @jihoonson @gianm @jon-wei

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.

This class was refactored extensively in #6370 and then renamed in #7306 to SqlSegmentsMetadataManager. So this issue might not still exist. Have you been able to take a look at the new code?

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.

Which for loop are you referring to? Can you link here? Also, there has been a pretty big refactoring for this class. Do you still see the same issue?

riovic918data pushed a commit to riovic918data/druid that referenced this pull request Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants