-
Notifications
You must be signed in to change notification settings - Fork 511
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
HDDS-11746. Support arbitrary schemas as an option in ozone debug ldb. #7652
base: master
Are you sure you want to change the base?
Conversation
As with any CLI command, please include robot tests. |
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.
Thanks @Tejaskriya for the patch.
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java
Outdated
Show resolved
Hide resolved
…f which should exist
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.
Thanks @Tejaskriya for updating the patch, LGTM. Few minor improvements suggested on second look.
hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java
Outdated
Show resolved
Hide resolved
Thanks for the review @adoroszlai, I have improved the code following your suggestions! |
@errose28 @adoroszlai I think renaming the option to |
If you would like to change it, I prefer
|
@swamirishi this is an implementation of your earlier suggestion in case you want to check it out |
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.
@Tejaskriya Thanks for working on the patch but I believe the requirement for this kind of generic tooling would be better if we can open random rocksdb tables as long as we have a way to decode value in the classpath.
@@ -43,11 +43,19 @@ public class RDBParser implements DebugSubcommand { | |||
description = "Database File Path") | |||
private String dbPath; | |||
|
|||
@CommandLine.Option(names = {"--schema"}, |
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.
Instead of taking a schema, I would rather take an input of codec to decode the value of a column family. For instance if I want to read snapshot diff DB, the DB doesn't have DB definition since we create new column families on the fly based on the snapdiff job.
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.
We should open a rocksdb in read only mode and decode value based on the ValueCodec
What changes were proposed in this pull request?
ozone debug ldb
automatically detects which schema to use for the provided DB based on its name. However, there may be cases where the tool has not been updated to handle a different DB or format. To support these cases before full support is added, a --schema= option is added that allows passing a java class present in the classpath which implements DBDefinition to use as the schema for displaying a provided DB.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11746
How was this patch tested?
Added unit test in TestDBDefinitionFactory and integration test in TestLDBCli.
Also added a test in TestDBDefinitionFactory to ensure any DBDefinitions added in the future will be usable through the newly introduced option (i.e., the requires constructor is present).