You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "fabio-rizzo-01 (via GitHub)" <gi...@apache.org> on 2025/07/29 13:37:47 UTC
[PR] Retry logic only added for update of namespace properties. Added the new methods in PolarisMetaStoreManger and retry logic implemented only for TransactionMetastoreManagerImpl. [polaris]
fabio-rizzo-01 opened a new pull request, #2206:
URL: https://github.com/apache/polaris/pull/2206
# This is the 1st commit message:
updating indentation
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
--
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] Add retry logic to handle concurrent updates for namespace properties [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2206:
URL: https://github.com/apache/polaris/pull/2206#issuecomment-3239681428
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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] Add retry logic to handle concurrent updates for namespace properties [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #2206: Add retry logic to handle concurrent updates for namespace properties
URL: https://github.com/apache/polaris/pull/2206
--
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] #2183 Add retry logic to handle concurrent updates for namespace properties [polaris]
Posted by "fabio-rizzo-01 (via GitHub)" <gi...@apache.org>.
fabio-rizzo-01 commented on code in PR #2206:
URL: https://github.com/apache/polaris/pull/2206#discussion_r2241813933
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -247,6 +251,48 @@ EntityResult updateEntityPropertiesIfNotChanged(
EntitiesResult updateEntitiesPropertiesIfNotChanged(
@Nonnull PolarisCallContext callCtx, @Nonnull List<EntityWithPath> entities);
+ /**
+ * Update some properties of this entity, and we will retry the update up to {@code numOfRetries}
+ * times, assuming it can still be resolved the same way and itself has not changed. If this is
+ * not the case the last update wins, and it will update both the internal and visible properties
+ * and return {@link EntityResult}. if everything fails we will return {@link EntityResult} with
+ * an empty entity and an error code {@link BaseResult.ReturnStatus}
+ * TARGET_ENTITY_CONCURRENTLY_MODIFIED.
+ *
+ * @param callCtx call context
+ * @param catalogPath path to that entity. Could be null if this entity is top-level
+ * @param entity entity to update, cannot be null
+ * @return the entity we updated or null if the client should retry
+ */
+ @Nonnull
+ EntityResult retryUpdateEntityProperties(
+ @Nonnull PolarisCallContext callCtx,
+ @Nullable List<PolarisEntityCore> catalogPath,
+ @Nonnull PolarisBaseEntity entity,
+ @NotNull Map<String, String> properties,
+ int numOfRetries);
Review Comment:
thank you for the feedback. I was initially unsure about the appropriate level for implementing the retry logic. I considered placing it in the IcebergCatalog but ultimately decided it made more sense at the metastore level. I'll update the pull request to move the logic to the IcebergCatalog. If anyone has additional suggestions, please feel free to comment
--
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] #2183 Add retry logic to handle concurrent updates for namespace properties [polaris]
Posted by "fabio-rizzo-01 (via GitHub)" <gi...@apache.org>.
fabio-rizzo-01 commented on PR #2206:
URL: https://github.com/apache/polaris/pull/2206#issuecomment-3132818715
@dimas-b please have a look when you have time to see if this is going in the direction you were suggesting?
--
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] #2183 Add retry logic to handle concurrent updates for namespace properties [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2206:
URL: https://github.com/apache/polaris/pull/2206#discussion_r2240813084
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -247,6 +251,48 @@ EntityResult updateEntityPropertiesIfNotChanged(
EntitiesResult updateEntitiesPropertiesIfNotChanged(
@Nonnull PolarisCallContext callCtx, @Nonnull List<EntityWithPath> entities);
+ /**
+ * Update some properties of this entity, and we will retry the update up to {@code numOfRetries}
+ * times, assuming it can still be resolved the same way and itself has not changed. If this is
+ * not the case the last update wins, and it will update both the internal and visible properties
+ * and return {@link EntityResult}. if everything fails we will return {@link EntityResult} with
+ * an empty entity and an error code {@link BaseResult.ReturnStatus}
+ * TARGET_ENTITY_CONCURRENTLY_MODIFIED.
+ *
+ * @param callCtx call context
+ * @param catalogPath path to that entity. Could be null if this entity is top-level
+ * @param entity entity to update, cannot be null
+ * @return the entity we updated or null if the client should retry
+ */
+ @Nonnull
+ EntityResult retryUpdateEntityProperties(
+ @Nonnull PolarisCallContext callCtx,
+ @Nullable List<PolarisEntityCore> catalogPath,
+ @Nonnull PolarisBaseEntity entity,
+ @NotNull Map<String, String> properties,
+ int numOfRetries);
Review Comment:
I'm not sure adding another method to `PolarisMetaStoreManager` for `numOfRetries` is the best approach.
For most entities, updates should be based on the specific previous version of the entity, so retrying at the MetaStore level will violate the normal optimistic locking approach.
Namespaces are special only from the Iceberg REST Catalog API perspective. That API is defined such that Namespace property updates could be executed in the "last write wins" manner. However, in that case, I believe, retries should be handled in `IcebergCatalog` (or at a higher level, perhaps).
--
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] #2183 Add retry logic to handle concurrent updates for namespace properties [polaris]
Posted by "fabio-rizzo-01 (via GitHub)" <gi...@apache.org>.
fabio-rizzo-01 commented on code in PR #2206:
URL: https://github.com/apache/polaris/pull/2206#discussion_r2241813933
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -247,6 +251,48 @@ EntityResult updateEntityPropertiesIfNotChanged(
EntitiesResult updateEntitiesPropertiesIfNotChanged(
@Nonnull PolarisCallContext callCtx, @Nonnull List<EntityWithPath> entities);
+ /**
+ * Update some properties of this entity, and we will retry the update up to {@code numOfRetries}
+ * times, assuming it can still be resolved the same way and itself has not changed. If this is
+ * not the case the last update wins, and it will update both the internal and visible properties
+ * and return {@link EntityResult}. if everything fails we will return {@link EntityResult} with
+ * an empty entity and an error code {@link BaseResult.ReturnStatus}
+ * TARGET_ENTITY_CONCURRENTLY_MODIFIED.
+ *
+ * @param callCtx call context
+ * @param catalogPath path to that entity. Could be null if this entity is top-level
+ * @param entity entity to update, cannot be null
+ * @return the entity we updated or null if the client should retry
+ */
+ @Nonnull
+ EntityResult retryUpdateEntityProperties(
+ @Nonnull PolarisCallContext callCtx,
+ @Nullable List<PolarisEntityCore> catalogPath,
+ @Nonnull PolarisBaseEntity entity,
+ @NotNull Map<String, String> properties,
+ int numOfRetries);
Review Comment:
thank you for the feedback. I was initially unsure about the appropriate level for implementing the retry logic. I considered placing it in the IcebergCatalog but ultimately decided it made more sense at the metastore level. I'll update the pull request to move the logic to the IcebergCatalog or at an higher level. If anyone has additional suggestions, please feel free to comment
--
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