-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate Legacy Metadata Format to new Role-Based Format #1081
base: main
Are you sure you want to change the base?
Conversation
Motivation: In line#1060, role-based authentication was introduced, changing the structure of metadata to support roles (READ, WRITE, ADMIN). Modifications: - Implemented a migration plugin to convert legacy metadata into the new role-based format. Result: - The legacy metadata format is migrated to new role-based format.
} catch (Exception ex) { | ||
// No need to rollback because the migration is not committed. Just log the error. | ||
logger.warn("Failed to migrate metadata roles of {}", name, ex); | ||
return UnmodifiableFuture.completedFuture(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question)
- If a metadata is already migrated, will a
RedundantChangeException
be thrown? - If an exception is thrown, would it be a better idea to log and continue instead of returning? Whether the metadata was migrated or not doesn't seem to affect the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a metadata is already migrated, will a RedundantChangeException be thrown?
Just the head revision is returned.
centraldogma/server/src/main/java/com/linecorp/centraldogma/server/metadata/RepositorySupport.java
Lines 128 to 132 in a813dcd
if (peeled instanceof RedundantChangeException) { | |
final Revision revision = ((RedundantChangeException) peeled).headRevision(); | |
assert revision != null; | |
return revision; | |
} |
If an exception is thrown, would it be a better idea to log and continue instead of returning? Whether the metadata was migrated or not doesn't seem to affect the behavior.
Yeah, we can probably do that. Let me make a change. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍👍
for (Project project : context.projectManager().list().values()) { | ||
final String name = project.name(); | ||
try { | ||
metadataService.migrateMetadata(name).join(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q) The blocking code will prevent other plugins from executing. Is that intentional?
centraldogma/server/src/main/java/com/linecorp/centraldogma/server/PluginGroup.java
Lines 186 to 187 in 91ad720
plugin -> plugin.start(arg) | |
.thenAccept(unused -> logger.info("Plugin started: {}", plugin)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that intentional?
That is true. This approach is inspired by the mirroring migration job:
https://github.com/line/centraldogma/pull/880/files#diff-52b26e4b2cfaf28da0494ec3a92464b4b609594aeccb0a6f3c74d49a7454cf5cR135
context.commandExecutor()); | ||
final Stopwatch stopwatch = Stopwatch.createStarted(); | ||
for (Project project : context.projectManager().list().values()) { | ||
final String name = project.name(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a migration log for each project so we can identify the progress and investegate when something goes wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a relatively simple migration so I thought just leaving the warning log was enough. 😄
Motivation:
In #1060, role-based authentication was introduced, changing the structure of metadata to support roles (READ, WRITE, ADMIN).
Modifications:
Result: