Copilot commented on code in PR #12350:
URL: https://github.com/apache/gravitino/pull/12350#discussion_r3710981965
##########
core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java:
##########
@@ -138,7 +138,7 @@ public static MetalakePO updateMetalakePOWithVersion(
MetalakePO oldMetalakePO, BaseMetalake newMetalake) {
Long lastVersion = oldMetalakePO.getLastVersion();
// Will set the version to the last version + 1 when having some fields
need be multiple version
Review Comment:
The comment describing version behavior is unclear/grammatically incorrect
and no longer matches the implementation (the code now always increments the
version). Please reword it to reflect OCC semantics so future readers don’t
misinterpret `lastVersion` vs `currentVersion`.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SchemaMetaService.java:
##########
@@ -230,15 +231,19 @@ public <E extends Entity & HasIdentifier> SchemaEntity
updateSchema(
AtomicInteger updateResult = new AtomicInteger(0);
try {
SessionUtils.doMultipleWithCommit(
- () ->
- updateResult.set(
- SessionUtils.getWithoutCommit(
- SchemaMetaMapper.class,
- mapper ->
- ops.updatePO(
- mapper,
-
POConverters.updateSchemaPOWithVersion(oldSchemaPO, newEntity),
- oldSchemaPO))),
+ () -> {
+ updateResult.set(
+ SessionUtils.getWithoutCommit(
+ SchemaMetaMapper.class,
+ mapper ->
+ ops.updatePO(
+ mapper,
+
POConverters.updateSchemaPOWithVersion(oldSchemaPO, newEntity),
+ oldSchemaPO)));
+ if (updateResult.get() == 0) {
+ throw optimisticLockException(identifier);
+ }
Review Comment:
`updateSchema` now throws `OptimisticLockException` when the version-CAS
update affects 0 rows, but there is no test asserting the service-level
conflict behavior for schemas. Please add a regression test that introduces a
competing update (or delete) and verifies `OptimisticLockException` is thrown
so the OCC contract is covered at the service layer.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/CatalogMetaService.java:
##########
@@ -229,15 +230,19 @@ public <E extends Entity & HasIdentifier> CatalogEntity
updateCatalog(
AtomicInteger updateResult = new AtomicInteger(0);
try {
SessionUtils.doMultipleWithCommit(
- () ->
- updateResult.set(
- SessionUtils.getWithoutCommit(
- CatalogMetaMapper.class,
- mapper ->
- mapper.updateCatalogMeta(
- POConverters.updateCatalogPOWithVersion(
- oldCatalogPO, newEntity,
oldCatalogPO.getMetalakeId()),
- oldCatalogPO))),
+ () -> {
+ updateResult.set(
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
+ mapper.updateCatalogMeta(
+ POConverters.updateCatalogPOWithVersion(
+ oldCatalogPO, newEntity,
oldCatalogPO.getMetalakeId()),
+ oldCatalogPO)));
+ if (updateResult.get() == 0) {
+ throw optimisticLockException(identifier);
+ }
+ },
Review Comment:
`updateCatalog` now throws `OptimisticLockException` when the version-CAS
update affects 0 rows, but there is no test asserting the service-level
conflict behavior for catalogs (similar to
`TestMetalakeMetaService.testAlterReportsOptimisticLockConflict`). Adding a
regression test that forces a competing update and then asserts
`OptimisticLockException` would ensure the new OCC contract is enforced
end-to-end.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/MetalakeMetaService.java:
##########
@@ -420,6 +425,22 @@ public boolean deleteMetalake(NameIdentifier ident,
boolean cascade) {
return true;
}
+ private void deleteMetalakeWithVersion(
+ NameIdentifier identifier, Long metalakeId, Long currentVersion) {
+ int deleted =
+ SessionUtils.getWithoutCommit(
+ MetalakeMetaMapper.class,
+ mapper -> mapper.softDeleteMetalakeMetaByMetalakeId(metalakeId,
currentVersion));
+ if (deleted == 0) {
+ throw optimisticLockException(identifier);
Review Comment:
`deleteMetalakeWithVersion` now throws `OptimisticLockException` when the
version-CAS delete affects 0 rows, but the test suite only covers the alter
conflict for metalakes. Please add a regression test that forces a stale delete
(e.g., competing update increments the version, then delete with the old
version) and asserts `OptimisticLockException`, to cover the new OCC delete
behavior.
--
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]
