What would you like to be improved?
FilesetMetaService.updateFileset has a potentially unsafe success path when checkNeedUpdateVersion is true. In that branch, the code runs insertFilesetVersions(...) and updateFilesetMeta(...) inside one transaction, but it does not check the row count returned by updateFilesetMeta(...). Instead it unconditionally sets updateResult = 1 and returns success.
Because updateFilesetMeta(...) uses optimistic matching on the old row values, it can return 0 if the metadata row changed between read and update. In that case, the method may still report success even though the fileset metadata update did not apply.
How should we improve?
Capture and validate the result of updateFilesetMeta(...) in the version-update branch as well. If the update affects 0 rows, treat it as an update conflict and fail the operation instead of forcing success.
One straightforward fix is to make the multi-step transaction return the metadata update count, then throw when the count is 0.
Here is a test to help:
@TestTemplate
public void testUpdateFilesetReturnsSuccessWhenVersionedMetaUpdateAffectsNoRows()
throws IOException {
String filesetName = GravitinoITUtils.genRandomName("tst_fs_conflict");
NameIdentifier filesetIdent =
NameIdentifier.of(metalakeName, catalogName, schemaName, filesetName);
FilesetEntity filesetEntity =
createFilesetEntity(
RandomIdGenerator.INSTANCE.nextId(),
NamespaceUtil.ofFileset(metalakeName, catalogName, schemaName),
filesetName,
AUDIT_INFO,
"/tmp");
FilesetMetaService.getInstance().insertFileset(filesetEntity, true);
AuditInfo conflictingAuditInfo =
AuditInfo.builder()
.withCreator("conflicting-updater")
.withCreateTime(Instant.now())
.build();
FilesetEntity updatedFilesetEntity =
FilesetEntity.builder()
.withId(filesetEntity.id())
.withName(filesetEntity.name())
.withNamespace(filesetEntity.namespace())
.withFilesetType(filesetEntity.filesetType())
.withStorageLocations(ImmutableMap.of(LOCATION_NAME_UNKNOWN, "/tmp-v2"))
.withComment("comment-v2")
.withProperties(ImmutableMap.of("version", "2"))
.withAuditInfo(
AuditInfo.builder()
.withCreator("expected-updater")
.withCreateTime(Instant.now())
.build())
.build();
FilesetEntity returnedEntity =
FilesetMetaService.getInstance()
.updateFileset(
filesetIdent,
e -> {
updateFilesetAuditInfo(filesetEntity.id(), conflictingAuditInfo);
return updatedFilesetEntity;
});
Assertions.assertEquals(updatedFilesetEntity, returnedEntity);
FilesetEntity persistedEntity =
FilesetMetaService.getInstance().getFilesetByIdentifier(filesetIdent);
Assertions.assertEquals(conflictingAuditInfo, persistedEntity.auditInfo());
Assertions.assertEquals("", persistedEntity.comment());
Assertions.assertNull(persistedEntity.properties());
Assertions.assertEquals("/tmp", persistedEntity.storageLocations().get(LOCATION_NAME_UNKNOWN));
Assertions.assertNotEquals(updatedFilesetEntity, persistedEntity);
Assertions.assertEquals(2, listFilesetVersions(filesetEntity.id()).size());
}
What would you like to be improved?
FilesetMetaService.updateFileset has a potentially unsafe success path when checkNeedUpdateVersion is true. In that branch, the code runs insertFilesetVersions(...) and updateFilesetMeta(...) inside one transaction, but it does not check the row count returned by updateFilesetMeta(...). Instead it unconditionally sets updateResult = 1 and returns success.
Because updateFilesetMeta(...) uses optimistic matching on the old row values, it can return 0 if the metadata row changed between read and update. In that case, the method may still report success even though the fileset metadata update did not apply.
How should we improve?
Capture and validate the result of updateFilesetMeta(...) in the version-update branch as well. If the update affects 0 rows, treat it as an update conflict and fail the operation instead of forcing success.
One straightforward fix is to make the multi-step transaction return the metadata update count, then throw when the count is 0.
Here is a test to help: