HDDS-11716. Address Incomplete Upgrade Scenario in Recon Upgrade Framework #7452
Conversation
There was a problem hiding this comment.
Thanks @ArafatKhan2198 for the patch. Overall changes LGTM, Few comments to take care. Pls check.
| try { | ||
| boolean recordExists = dslContext.fetchExists(dslContext.selectOne() | ||
| .from(DSL.table(RECON_SCHEMA_VERSION_TABLE_NAME))); | ||
| public void updateSchemaVersion(int newVersion, Connection conn) |
There was a problem hiding this comment.
this method doesn't throw SQLException
| */ | ||
| private void updateSchemaVersion(int newVersion) throws SQLException { | ||
| schemaVersionTableManager.updateSchemaVersion(newVersion); | ||
| private void updateSchemaVersion(int newVersion, Connection connection) throws SQLException { |
There was a problem hiding this comment.
this method doesn't throw SQLException.
| action.get().execute(scmFacade); | ||
| updateSchemaVersion(feature.getVersion()); | ||
| LOG.info("Feature versioned {} finalized successfully.", feature.getVersion()); | ||
| try (Connection connection = scmFacade.getDataSource() |
There was a problem hiding this comment.
Do we need to open connection everytime for every feature ? Can we use same connection for every feature ?
There was a problem hiding this comment.
IMO, every feature as once a feature is upgraded, it should not be rollbacked due to crash in next feature.
| LOG.info("Feature versioned {} finalized successfully.", | ||
| feature.getVersion()); | ||
| } catch (Exception e) { | ||
| // Rollback any pending change`s due to failure |
There was a problem hiding this comment.
This comment as well as log message doesn't fit well as we are not rolling back anything. It is just that it will not be committed.
| @@ -112,8 +112,8 @@ public void testFinalizeLayoutFeaturesWithMockedValues() throws SQLException { | |||
| ReconStorageContainerManagerFacade.class)); | |||
|
|
|||
| // Verify that schema versions are updated for our custom features | |||
| verify(schemaVersionTableManager, times(1)).updateSchemaVersion(1); | |||
| verify(schemaVersionTableManager, times(1)).updateSchemaVersion(2); | |||
| verify(schemaVersionTableManager, times(1)).updateSchemaVersion(1, null); | |||
There was a problem hiding this comment.
Can you write a test case to verify in case of edge case when exception being thrown.
What changes were proposed in this pull request?
This PR enhances the ReconLayoutVersionManager to ensure upgrade actions and schema version updates occur together in a single transaction, preventing inconsistent states.
Key Changes:
Transactional Handling: Upgrade actions and schema updates are wrapped in a single transaction.
Schema updates only commit if the action succeeds; otherwise, changes are rolled back.
Enhanced updateSchemaVersion: Modified to accept a Connection for transaction consistency, aligning with the transactional scope of finalizeLayoutFeatures.
System Behavior with Latest Changes:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11716
How was this patch tested?