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 1 commit
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
24 changes: 13 additions & 11 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 @@ -65,6 +65,7 @@
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -950,14 +951,7 @@ public DataSegment map(int index, ResultSet r, StatementContext ctx) throws SQLE
}
);

if (segments == null || segments.isEmpty()) {
log.info("No segments found in the database!");
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
Expand All @@ -967,10 +961,18 @@ 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.
dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments(
Iterables.filter(segments, Objects::nonNull), // Filter corrupted entries (see above in this method).
dataSourceProperties
);

ImmutableMap<String, String> dataSourceProperties = createDefaultDataSourceProperties();
if (segments == null || segments.isEmpty()) {
log.info("No segments found in the database!");
dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments(Collections.emptyList(), dataSourceProperties);
Copy link
Member

@leventov leventov Jul 19, 2019

Choose a reason for hiding this comment

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

I don't know how segments == null is possible here, but if dataSourcesSnapshot is already non-null, probably this assignment should be skipped. The cause may be intermittent database unavailability or failure. This condition should be logged.

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.

Yea, I was not able to observe segments being null in my experiments, which mostly consisted of killing and restarting the mysql i was testing against. It doesn't look like it should be able to be null, the javadoc for the Query.list method

     * Executes the select
     * <p/>
     * Will eagerly load all results
     *
     * @throws org.skife.jdbi.v2.exceptions.UnableToCreateStatementException
     *                            if there is an error creating the statement
     * @throws org.skife.jdbi.v2.exceptions.UnableToExecuteStatementException
     *                            if there is an error executing the statement
     * @throws org.skife.jdbi.v2.exceptions.ResultSetException if there is an error dealing with the result set

makes it look like it's either going to make a list or throw an exception.

If we are confident it shouldn't happen then it should just be removed i think, but if still unsure it might make more sense to handle segments == null separate and log and probably not update the snapshot even if it's still null, because it's an unexpected condition that doesn't necessarily mean the same thing as a truly empty segments table. Thoughts?

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.

I guess it's harmless to put an if (segments == null) block that does a log.wtf and returns early, will go ahead and modify to do that just in case.

} 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
);
}
}

private static ImmutableMap<String, String> createDefaultDataSourceProperties()
Expand Down