fix npe with sql metadata manager polling and empty database #8106
Conversation
| ImmutableMap<String, String> dataSourceProperties = createDefaultDataSourceProperties(); | ||
| if (segments == null || segments.isEmpty()) { | ||
| log.info("No segments found in the database!"); | ||
| dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments(Collections.emptyList(), dataSourceProperties); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
|
||
| if (segments == null || segments.isEmpty()) { | ||
| log.info("No segments found in the database!"); | ||
| if (segments == null) { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Preconditions check makes sense to me, to control the error messaging here instead of allowing the NPE to happen downstream, will update
There was a problem hiding this comment.
Er, actually this is just in the poll, i guess the NPE would still potentially happen, regardless that approach seems cleaner.
| // effect of a segment mark call reflected in MetadataResource API calls. | ||
|
|
||
| ImmutableMap<String, String> dataSourceProperties = createDefaultDataSourceProperties(); | ||
| if (segments.isEmpty()) { |
There was a problem hiding this comment.
This part is the fix, right? (Continuing on even if segments is empty?)
It looks good to me.
|
Thanks for the review! |
4 participants
Description
This fixes a null pointer exception that occurs in numerous places when polling for segments while having an empty metadata database after the changes of #7653. I'm not certain if this is the correct fix since previously the snapshot would remain null until there were segments available during a poll, but it resolves the issue for me at least, and doesn't appear to have any ill side-effects.
etc.
This PR has: