devmadhuu commented on code in PR #7452:
URL: https://github.com/apache/ozone/pull/7452#discussion_r1853282119
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/upgrade/ReconLayoutVersionManager.java:
##########
@@ -123,10 +142,13 @@ protected List<ReconLayoutFeature>
getRegisteredFeatures() {
/**
* Updates the Metadata Layout Version (MLV) in the database after
finalizing a feature.
+ * This method uses the provided connection to ensure transactional
consistency.
+ *
* @param newVersion The new Metadata Layout Version (MLV) to set.
+ * @param connection The database connection to use for the update operation.
*/
- private void updateSchemaVersion(int newVersion) throws SQLException {
- schemaVersionTableManager.updateSchemaVersion(newVersion);
+ private void updateSchemaVersion(int newVersion, Connection connection)
throws SQLException {
Review Comment:
this method doesn't throw SQLException.
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/upgrade/TestReconLayoutVersionManager.java:
##########
@@ -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);
Review Comment:
Can you write a test case to verify in case of edge case when exception
being thrown.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconSchemaVersionTableManager.java:
##########
@@ -71,30 +72,27 @@ public int getCurrentSchemaVersion() throws SQLException {
*
* @param newVersion The new version to set.
*/
- public void updateSchemaVersion(int newVersion) throws SQLException {
- try {
- boolean recordExists = dslContext.fetchExists(dslContext.selectOne()
- .from(DSL.table(RECON_SCHEMA_VERSION_TABLE_NAME)));
+ public void updateSchemaVersion(int newVersion, Connection conn)
Review Comment:
this method doesn't throw SQLException
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/upgrade/ReconLayoutVersionManager.java:
##########
@@ -88,10 +89,27 @@ public void
finalizeLayoutFeatures(ReconStorageContainerManagerFacade scmFacade)
// Fetch only the FINALIZE action for the feature
Optional<ReconUpgradeAction> action =
feature.getAction(ReconUpgradeAction.UpgradeActionType.FINALIZE);
if (action.isPresent()) {
- // Execute the upgrade action & update the schema version in the DB
- action.get().execute(scmFacade);
- updateSchemaVersion(feature.getVersion());
- LOG.info("Feature versioned {} finalized successfully.",
feature.getVersion());
+ try (Connection connection = scmFacade.getDataSource()
Review Comment:
Do we need to open connection everytime for every feature ? Can we use same
connection for every feature ?
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/upgrade/ReconLayoutVersionManager.java:
##########
@@ -88,10 +89,27 @@ public void
finalizeLayoutFeatures(ReconStorageContainerManagerFacade scmFacade)
// Fetch only the FINALIZE action for the feature
Optional<ReconUpgradeAction> action =
feature.getAction(ReconUpgradeAction.UpgradeActionType.FINALIZE);
if (action.isPresent()) {
- // Execute the upgrade action & update the schema version in the DB
- action.get().execute(scmFacade);
- updateSchemaVersion(feature.getVersion());
- LOG.info("Feature versioned {} finalized successfully.",
feature.getVersion());
+ try (Connection connection = scmFacade.getDataSource()
+ .getConnection()) {
+ connection.setAutoCommit(
+ false); // Turn off auto-commit for transactional control
+
+ // Update the schema version in the database
+ updateSchemaVersion(feature.getVersion(), connection);
+
+ // Execute the upgrade action
+ action.get().execute(scmFacade);
+
+ // Commit the transaction only if both operations succeed
+ connection.commit();
+ LOG.info("Feature versioned {} finalized successfully.",
+ feature.getVersion());
+ } catch (Exception e) {
+ // Rollback any pending change`s due to failure
Review 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.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
