Remove hostnamePatterns from credential files #1030
Conversation
Motivation: We decided to repurpose the `MirrorCredential` to manage all repository credentials, not just those specific to mirroring. The hostnamePatterns property, which was specific to mirroring, is no longer necessary. With the introduction of a dedicated mirroring API, hostnamePatterns can be safely removed. Modifications: - Added a job that removes the `hostnamePatterns` property from existing credential files. This prepares the system for the complete removal of this property in the next PR. Result: - The `hostnamePatterns` property is now deprecated and will be removed entirely in the subsequent PR.
Motivation: We decided to repurpose the `MirrorCredential` to manage all repository credentials, not just those specific to mirroring. To reflect this role, we should remove `Mirror` prefix from the class name. Caveat: This commit must be deployed to central dogma replicas after applying the changed from line#1030. Modifications: - Renamed `MirroringCredential` to `Credential` and moved its package. - Removed `hostnamePatterns` property in `Credential`. Result: - The renamed `Credential` class can now be used for managing various types of repository credentials, beyond just mirroring.
| project.name()); | ||
| } | ||
|
|
||
| commandExecutor.execute(Command.forcePush(Command.push( |
There was a problem hiding this comment.
Should we skip pushing this commit if changes is empty?
| try { | ||
| new MirroringMigrationService(projectManager, commandExecutor).migrate(); |
There was a problem hiding this comment.
Note: I understood that it will be mentioned in the release notes clearly that #880 (0.67.0) should be released before this version
Note: I think other parties hosting their own CentralDogma servers may have difficulty upgrading with the recent versions. Ideally, we may want to consider in the future:
- Add warning logs to check if
hostnamePatternsis indeed not being used, and guide users to migrate (possibly in the following location)
- After a couple of releases, remove the
hostnamePatternsfield and data
There was a problem hiding this comment.
I understood that it will be mentioned in the release notes clearly that #880 (0.67.0) should be released before this version
That is correct. 👍
Ideally, we may want to consider in the future:
That's a good idea. 👍
Since the PR hasn't been merged yet, we can still proceed with your suggestion. However, I opted for this approach because:
- This change isn't related to a widely-used client feature.
- We can provide guidance to the server operators, who are fewer in number.
- (Personally) I'd like to wrap up the mirror-related tasks as quickly as possible."
| if (commandExecutor instanceof ZooKeeperCommandExecutor) { | ||
| logger.debug("Waiting for 30 seconds to make sure that all cluster have been notified of the " + | ||
| "read-only mode ..."); | ||
| Thread.sleep(30000); |
There was a problem hiding this comment.
Note: For the record, I don't think removing hostnames is a critical operation, and can be considered more of a data cleanup.
The current approach requires that 1) writes are blocked (which requires us to announce operations) and 2) maintainers also need to clear time for monitoring and possible operations.
Given that mirror configs are rarely modified, perhaps just pushing with a specific revision/retrying is safer.
There was a problem hiding this comment.
For the record, I don't think removing hostnames is a critical operation, and can be considered more of a data cleanup.
That is correct. 👍
I've implemented it similarly to how MirroringMigrationService is done, but it might be a bit excessive.
|
|
||
| void start() throws Exception { | ||
| // Enter read-only mode. | ||
| commandExecutor.execute(Command.updateServerStatus(ServerStatus.REPLICATION_ONLY)) |
There was a problem hiding this comment.
Question) Will this code path be executed for every leader change?
Given that we already had several issues with readonly mode, I prefer that the command isn't issued when we're not aware.
e.g. servers randomly go through leader election during the weekend, and servers go into readonly mode without us being aware.
There was a problem hiding this comment.
Question) Will this code path be executed for every leader change?
It shouldn't. 😓 Let me add a file to prevent it.
Motivation: We decided to repurpose the `MirrorCredential` to manage all repository credentials, not just those specific to mirroring. To reflect this role, we should remove `Mirror` prefix from the class name. Caveat: This commit must be deployed to central dogma replicas after applying the changes from #1030. Modifications: - Renamed `MirroringCredential` to `Credential` and moved its package. - Removed `hostnamePatterns` property in `Credential`. Result: - The renamed `Credential` class can now be used for managing various types of repository credentials, beyond just mirroring.
Motivation:
We decided to repurpose the
MirrorCredentialto manage all repository credentials, not just those specific to mirroring. The hostnamePatterns property, which was specific to mirroring, is no longer necessary. With the introduction of a dedicated mirroring API, hostnamePatterns can be safely removed.Modifications:
hostnamePatternsproperty from existing credential files. This prepares the system for the complete removal of this property in the next PR.MirroringMigrationServiceto avoid conflict while executing the job.Result:
hostnamePatternsproperty is now deprecated and will be removed entirely in the subsequent PR.