[admin-tool][controller]Validate isMigrating flag during store deletion that happens as part of abortMigration #1436
Conversation
There was a problem hiding this comment.
Why do we need this change?
There was a problem hiding this comment.
In order to write unit tests for static methods using Mockito, we need to add the mockito-inline dependency. This is because the default mock maker used by Mockito, which is SubclassByteBuddyMockMaker, does not support the creation of static mocks. Without this dependency, attempting to mock static methods will result in the following exception:
org.mockito.exceptions.base.MockitoException:
The used MockMaker SubclassByteBuddyMockMaker does not support the creation of static mocks.
Additionally, our da-vinci-client tests also include a file located at clients/da-vinci-client/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker. The org.mockito.plugins.MockMaker file specifies the mock maker to be used by Mockito. By adding the mockito-inline dependency, we ensure that Mockito uses the appropriate mock maker that supports static method mocking.
| void deleteStore( | ||
| String clusterName, | ||
| String storeName, | ||
| boolean abortMigratingStore, |
There was a problem hiding this comment.
Can we rename abortMigratingStore to isAbortMigrationCleanup?
| @@ -1042,17 +1042,27 @@ private void configureNewStore(Store newStore, VeniceControllerClusterConfig con | |||
| public void deleteStore( | |||
| String clusterName, | |||
| String storeName, | |||
| boolean abortStoreMigrating, | |||
There was a problem hiding this comment.
Same here: Can we rename abortMigratingStore to isAbortMigrationCleanup?
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java
Show resolved
Hide resolved
| } | ||
| if (abortStoreMigrating && store != null && !store.isMigrating()) { | ||
| LOGGER.error( | ||
| "Delete store: {} is triggered by --abort-migration command, however its migrating flag " |
There was a problem hiding this comment.
Can we rephrase it something like this?
LOGGER.warn("Deletion of store: {} in cluster: {} was issued as part of abort migration resource cleanup, but the store's isMigrating flag is false. Please ensure the store's isMigrating flag is set to true in the destination cluster before issuing the delete command to prevent accidental deletion of shared resources", storeName, clusterName);There was a problem hiding this comment.
Sure, I have updated the logger warning message as suggested. However, I noticed that the parameter in the Venice Zookeeper store file is named migrating instead of isMigrating. Therefore, I changed the message to use "store's migrating flag" instead of "store's isMigrating flag" to avoid confusion. Please let me know if my understanding is incorrect.
...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java
Show resolved
Hide resolved
| @@ -7,6 +7,7 @@ public class ControllerApiConstants { | |||
| public static final String SOURCE_GRID_FABRIC = "source_grid_fabric"; | |||
| public static final String BATCH_JOB_HEARTBEAT_ENABLED = "batch_job_heartbeat_enabled"; | |||
|
|
|||
| public static final String ABORT_MIGRATION_ENABLED = "abort_migration_enabled"; | |||
There was a problem hiding this comment.
Can we rename this to
public static final String IS_ABORT_MIGRATION_CLEANUP = "is_abort_migration_cleanup";
...s/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoreRoutesTest.java
Show resolved
Hide resolved
...ices/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java
Outdated
Show resolved
Hide resolved
...ices/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java
Show resolved
Hide resolved
ranwang2024
force-pushed
the
ranwang2024/validate_isMigrating_abort_migration
branch
from
9388ca1 to
908d262
Compare
…as part of abortMigration
ranwang2024
force-pushed
the
ranwang2024/validate_isMigrating_abort_migration
branch
from
908d262 to
452fabb
Compare
| Mockito.verify(srcControllerClient, times(0)).discoverCluster(storeName); | ||
| Mockito.verify(srcControllerClient, times(0)).abortMigration(storeName, dstCluster); // verify dest client is | ||
| // called with true flag | ||
| // and store name as |
There was a problem hiding this comment.
Can you move all of these comments on the separate lines? Maybe before verify statements?
| return deleteStore(storeName, false); | ||
| } | ||
|
|
||
| public TrackableControllerResponse deleteStore(String storeName, boolean isAbortMigratingStore) { |
There was a problem hiding this comment.
Should we change isAbortMigratingStore to isAbortMigrationCleanup?
Validate isMigrating flag during store deletion that happens as part of abortMigration
Summary, imperative, start upper case, don't end with a period
When aborting a migration, store deletion should always validate the isMigrating flag. Deletion should only proceed if this flag is set to true. Currently, this validation happens only on the client side (admin tool) and only for source cluster configs, which is insufficient.
To address this, the delete store command sent to the controller should include a flag indicating the need to check the isMigrating status in destination clusters.
This change adds validation to both the parent controller and the child controller, ensuring that the RT topic is not accidentally deleted when aborting a store migration.
How was this PR tested?
This PR was tested by writing and including unit tests and integration tests within the PR itself. These tests were then executed successfully to ensure the changes work as intended.
Does this PR introduce any user-facing changes?
If the
isMigratingflag for a store is set to false during the abortion of migration in the Parent Controller, the deletion of the destination store will not be initiated. Instead, a delete response with an error message—'Store {storeName}'s migrating flag is false. It is not safe to delete a store assumed to be migrating without the migrating flag set to true.'—will be returned, accompanied by the ErrorType.INVALID_CONFIG. If this situation occurs in the Child Controller, the RT topic will not be deleted, and instead of throwing an exception, an error message will be logged.