eric-maynard commented on code in PR #585:
URL: https://github.com/apache/polaris/pull/585#discussion_r1904429309
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -554,14 +554,27 @@ private String resolveNamespaceLocation(Namespace
namespace, Map<String, String>
.map(
entity -> {
if (entity.getType().equals(PolarisEntityType.CATALOG)) {
- return CatalogEntity.of(entity).getDefaultBaseLocation();
+ CatalogEntity catEntity = CatalogEntity.of(entity);
+ String catalogDefaultBaseLocation =
catEntity.getDefaultBaseLocation();
+ if (catalogDefaultBaseLocation == null) {
+ LOGGER.warn(
+ "Tried to resolve location with catalog with null
default base location. Catalog = {}",
+ catEntity);
+ }
+ return catalogDefaultBaseLocation;
Review Comment:
I think there was previously a bug where we were setting the base location
to null; it could conceivably happen again. Agreed that this should be in
`checkNotNull` or something similar though
> It seems like elsewhere in the code we're assuming there aren't nulls when
calling such methods.
Yes, I think we lean a little too heavily on the non-functional @nonnull
annotation unfortunately. Adding more lightweight checks for this around is
definitely a good idea
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
