Skip to content

fix npe with sql metadata manager polling and empty database #8106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 21, 2019
Merged

fix npe with sql metadata manager polling and empty database #8106

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0917dcd
fix npe with sql metadata manager polling and empty database
clintropolis Jul 18, 2019
f176503
treat null segments separately
clintropolis Jul 19, 2019
5a4d430
use preconditions check
clintropolis Jul 19, 2019
67775c2
add test
clintropolis Jul 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,15 +950,11 @@ public DataSegment map(int index, ResultSet r, StatementContext ctx) throws SQLE
}
);

if (segments == null || segments.isEmpty()) {
log.info("No segments found in the database!");
if (segments == null) {
Copy link
Contributor

@gianm gianm Jul 19, 2019

Choose a reason for hiding this comment

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

This code is pointless, since .list() above cannot return null and inReadOnlyTransaction will not convert a nonnull return to null. Might as well remove it, or include a Preconditions.checkNotNull or something (throw an exception on the unanticipated null rather than returning early and leaving the snapshot null).

Copy link
Member Author

@clintropolis clintropolis Jul 19, 2019

Choose a reason for hiding this comment

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

Preconditions check makes sense to me, to control the error messaging here instead of allowing the NPE to happen downstream, will update

Copy link
Member Author

@clintropolis clintropolis Jul 19, 2019

Choose a reason for hiding this comment

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

Er, actually this is just in the poll, i guess the NPE would still potentially happen, regardless that approach seems cleaner.

log.wtf("Observed 'null' when polling segments from the db, aborting snapshot update.");
return;
}

log.info("Polled and found %,d segments in the database", segments.size());

ImmutableMap<String, String> dataSourceProperties = createDefaultDataSourceProperties();

// dataSourcesSnapshot is updated only here and the DataSourcesSnapshot object is immutable. If data sources or
// segments are marked as used or unused directly (via markAs...() methods in MetadataSegmentManager), the
// dataSourcesSnapshot can become invalid until the next database poll.
Expand All @@ -967,6 +963,13 @@ public DataSegment map(int index, ResultSet r, StatementContext ctx) throws SQLE
// segment mark calls in rapid succession. So the snapshot update is not done outside of database poll at this time.
// Updates outside of database polls were primarily for the user experience, so users would immediately see the
// effect of a segment mark call reflected in MetadataResource API calls.

ImmutableMap<String, String> dataSourceProperties = createDefaultDataSourceProperties();
if (segments.isEmpty()) {
Copy link
Contributor

@gianm gianm Jul 19, 2019

Choose a reason for hiding this comment

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

This part is the fix, right? (Continuing on even if segments is empty?)

It looks good to me.

log.info("No segments found in the database!");
} else {
log.info("Polled and found %,d segments in the database", segments.size());
}
dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments(
Iterables.filter(segments, Objects::nonNull), // Filter corrupted entries (see above in this method).
dataSourceProperties
Expand Down