Add log lines to help debug NPE in resolveLocationForPath #585
Conversation
d87zhang
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike,
ebyhr and
adutra
as code owners
d87zhang
force-pushed
the
log-lines-for-npe-resolveLocationForPath
branch
from
4de6855 to
8e75d9d
Compare
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java
Outdated
Show resolved
Hide resolved
| @@ -554,14 +554,27 @@ private String resolveNamespaceLocation(Namespace namespace, Map<String, String> | |||
| .map( | |||
| entity -> { | |||
| if (entity.getType().equals(PolarisEntityType.CATALOG)) { | |||
There was a problem hiding this comment.
Refactor suggestion: would the code be cleaner if we put these into a new method like
private static String baseLocation(PolarisEntity entity) {
if (entity.getType().equals(PolarisEntityType.CATALOG)) {
return CatalogEntity.of(entity).getDefaultBaseLocation();
} else {
String baseLocation =
entity.getPropertiesAsMap().get(PolarisEntityConstants.ENTITY_BASE_LOCATION);
if (baseLocation != null) {
return baseLocation;
} else {
return entity.getName();
}
}
}
There was a problem hiding this comment.
sounds good. Moved this logic to baseLocation helper!
| if (catalogDefaultBaseLocation == null) { | ||
| LOGGER.warn( | ||
| "Tried to resolve location with catalog with null default base location. Catalog = {}", | ||
| catEntity); | ||
| } | ||
| return catalogDefaultBaseLocation; |
There was a problem hiding this comment.
Should a catalog ever have a null base location? This feels like a case for PolarisDiagnostics.checkNotNull rather than a log statement.
There was a problem hiding this comment.
I'm not too familiar and don't know if this should ever not happen but sounds good to use PolarisDiagnostics.checkNotNull. I'm adding some code
callContext
.getPolarisCallContext().getDiagServices().checkNotNull
to get the diagnostic services to call checkNotNull though so please check if it's safe to do this or if we should be worried about NPEs.. It seems like elsewhere in the code we're assuming there aren't nulls when calling such methods.
| String entityName = entity.getName(); | ||
| if (entityName == null) { | ||
| LOGGER.warn( | ||
| "Tried to resolve location with entity without base location or name. entity = {}", | ||
| entity); | ||
| } | ||
| return entityName; |
There was a problem hiding this comment.
Same here - there should never be an entity without a name.
d87zhang
force-pushed
the
log-lines-for-npe-resolveLocationForPath
branch
from
04426e8 to
0589ae9
Compare
We could have NPEs in BasePolarisCatalog.resolveLocationForPath() today. This PR adds log lines to help debug in these situations.