You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "dennishuo (via GitHub)" <gi...@apache.org> on 2025/09/26 05:05:39 UTC
[PR] Fix a race condition in sendNotification where concurrent parent-namespace creation causes failures [polaris]
dennishuo opened a new pull request, #2693:
URL: https://github.com/apache/polaris/pull/2693
The semantics of the createNonExistingNamespaces method used during sendNotification were supposed to be "create if needed". However, the behavior ended up surfacing an AlreadyExistsException if multiple concurrent sendNotification attempts were made for a brand-new namespace (where the notifications may be different tables). This would cause a table sync to fail if a sibling table was being synced at the same time, even though the new table should successfully get created under the shared namespace.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a race condition in sendNotification where concurrent parent-namespace creation causes failures [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2693:
URL: https://github.com/apache/polaris/pull/2693#discussion_r2382853570
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
+ LOGGER
+ .atInfo()
Review Comment:
Why INFO? If we expect it to be a normal and expected call path, "informing" log readers every time it happens seems to be an overkill from the operations perspective. I propose debug (non-blocking)
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
+ LOGGER
+ .atInfo()
+ .setCause(aee)
+ .addKeyValue("namespace", namespace)
+ .log(
+ "Namespace already exists in createNonExistingNamespace; possible race condition");
Review Comment:
`possible race condition` sounds like it is a problem to address, but the intention of this PR seems to be to treat this case as a "normal" situation 🤔
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
+ LOGGER
+ .atInfo()
+ .setCause(aee)
+ .addKeyValue("namespace", namespace)
Review Comment:
Is it supposed to be referenced in the message?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a race condition in sendNotification where concurrent parent-namespace creation causes failures [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX merged PR #2693:
URL: https://github.com/apache/polaris/pull/2693
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a race condition in sendNotification where concurrent parent-namespace creation causes failures [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #2693:
URL: https://github.com/apache/polaris/pull/2693#discussion_r2382484669
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
Review Comment:
Currently we throw `AlreadyExistsException` if the result is not success without checking whether the return status is `ENTITY_ALREADY_EXIST`. Despite `ENTITY_ALREADY_EXIST` is the only error status thrown in existing metastore manager impl, furture development and other implementation may include other error status like `UNEXPECTED_ERROR`, where we do not want to ignore and log here.
I think we could add an additional check
https://github.com/apache/polaris/blob/19742cc20f4bc0b7e5a315a62f89c6085ad81b7d/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java#L518-L521
to only throw AlreadyExistsException when return status is that, otherwise throw a RuntimeException/IllegalState/...
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a race condition in sendNotification where concurrent parent-namespace creation causes failures [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2693:
URL: https://github.com/apache/polaris/pull/2693#discussion_r2382848065
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
Review Comment:
Good point!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a race condition in sendNotification where concurrent parent-namespace creation causes failures [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on code in PR #2693:
URL: https://github.com/apache/polaris/pull/2693#discussion_r2383452432
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
+ LOGGER
+ .atInfo()
+ .setCause(aee)
+ .addKeyValue("namespace", namespace)
Review Comment:
Yeah I actually normally default to the traditional format-string with curly-braces, but I think others have been pushing for the more modern structured logging convention so I'm been forcing myself to use it :)
Structured logging is indeed more beneficial for cases where logs sinks are good at having structured-log elements pulled out into separate semi-structured fields for efficient aggregations, etc.
The convention as I understand it is to make the main log message avoid any direct mention of the structured params: https://www.slf4j.org/manual.html
// using classical API
logger.debug("oldT={} newT={} Temperature changed.", oldT, newT);
// using fluent API
logger.atDebug().setMessage("Temperature changed.").addKeyValue("oldT", oldT).addKeyValue("newT", newT).log();
As opposed to `addArgument` which is for the traditional inline string-replacement into the human-friendly string, the `addKeyvalue` is standalone, and usually log outputters still show the addKeyValue fields conveniently in the log message, but for machine readers they are independent from the natural-language message.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
Review Comment:
Good catch, updated to throw `ServiceFailureException` if returnStatus isn't ENTITY_ALREADY_EXISTS.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
+ LOGGER
+ .atInfo()
+ .setCause(aee)
+ .addKeyValue("namespace", namespace)
+ .log(
+ "Namespace already exists in createNonExistingNamespace; possible race condition");
Review Comment:
Yeah good point, removed the second clause to make it less scary.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
+ LOGGER
+ .atInfo()
+ .setCause(aee)
+ .addKeyValue("namespace", namespace)
Review Comment:
Also added a comment above the log statement better elucidating the considerations
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
+ LOGGER
+ .atInfo()
Review Comment:
Yeah I was on the fence. It's one of those cases where it's "normal" as something that will happen time-to-time, but usually isn't deterministically triggered since here the *attempt* to create the namespace is only made if the immediate preceding call thinks the namespace is missing. So it would be fairly infrequent, only correlating with periods of high *parent-creation* contention (not just normal periods of high concurrency on an already-existing namespace).
I had even started `atWarn` but figured that's too alarming, so downgraded to info.
My thought is that if there are unforeseen bugs relating to the state of these "create if needed" namespaces they're more likely to be related to these collisions than to normal operation, so even if someone sets their log-viewer to exclude `DEBUG` level it'd be easy to eyeball a correlation between this log statement and whatever other buggy behavior comes up.
I also don't feel too strongly though so I'm happy to downgrade it if there's a preference for DEBUG despite this rationale .
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix a race condition in sendNotification where concurrent parent-namespace creation causes failures [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2693:
URL: https://github.com/apache/polaris/pull/2693#discussion_r2383576304
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2579,7 +2579,16 @@ private void createNonExistingNamespaces(Namespace namespace) {
Namespace parentNamespace = PolarisCatalogHelpers.getParentNamespace(nsLevel);
PolarisResolvedPathWrapper resolvedParent =
resolvedEntityView.getPassthroughResolvedPath(parentNamespace);
- createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ try {
+ createNamespaceInternal(nsLevel, Collections.emptyMap(), resolvedParent);
+ } catch (AlreadyExistsException aee) {
+ LOGGER
+ .atInfo()
Review Comment:
Fair enough. No need to change :)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org