HDDS-1469. Generate default configuration fragments based on annotations #773
Conversation
|
/cc @anuengineer This is the annotation processor. With a small modification in the OzoneConfiguration (to load all the generated fragments) we don't need to merge all the generated config files to one big ozone-default.xml |
|
💔 -1 overall
This message was automatically generated. |
anuengineer
left a comment
anuengineer
left a comment
There was a problem hiding this comment.
The code changes look excellent. I have some questions and Jenkins was not very happy, but in spirit, I am +1.
| Configuration.addDefaultResource("hdfs-default.xml"); | ||
| Configuration.addDefaultResource("hdfs-site.xml"); | ||
| Configuration.addDefaultResource("ozone-default.xml"); | ||
| Configuration.addDefaultResource("ozone-site.xml"); |
There was a problem hiding this comment.
Shouldn't we still allow this over-ride?
There was a problem hiding this comment.
We should. But in fact I didn't remove the possibility to use ozone-site.xml I just moved it.
The problem here is that the hadoop Configuration is not designed to be reused. We can use multiple Configuration.addDefaultResource(String) but there is no Configuration.addDefaultResource(URL) (and we can't create one as all the required methods are private :-( ).
With using configuration fragments we have multiple files with the same name (String) but with different URL (eg. jar://hadoop-ozone-ozone-manager.jar!/ozone-default-generated.xml).
To support this, but keep the same precedence I modified the configuration loading:
Configuration.addDefaultResource("hdfs-default.xml");
Configuration.addDefaultResource("hdfs-site.xml");
Configuration.addDefaultResource("ozone-default.xml");
conf.addResource("jar://ozone-manager.jar!/ozone-site-generated.xml");
conf.addResource("jar://storage-container-manager.jar!/ozone-site-generated.xml");
conf.addResource("ozone-site.xml");
The last three lines are in the constructor of OzoneConfiguration and as I would like to load ozone-site.xml at the end I moved it to there.
| loadDefaults(); | ||
| } | ||
|
|
||
| private void loadDefaults() { |
There was a problem hiding this comment.
Can I be a greedy pig and request that we also sort these keys (in another JIRA of course) before we write out the XML?
|
|
||
| @Config(key = "enabled", defaultValue = "true") | ||
| public void setEnabled(boolean enabled) { | ||
| this.enabled = enabled; |
There was a problem hiding this comment.
nit: what does client. enabled mean?
There was a problem hiding this comment.
This is just an example configuration (for unit tests) I invented a random boolean configuration. Name doesn't matter but I can use a more meaningful one. For example client.compression.enabled.
|
|
||
| @Config(key = "wait", type = ConfigType.TIME, timeUnit = | ||
| TimeUnit.SECONDS, defaultValue = "10m") | ||
| public void setWaitTime(long waitTime) { |
| */ | ||
| TimeUnit timeUnit() default TimeUnit.MILLISECONDS; | ||
|
|
||
| ConfigTag[] tags() default {ConfigTag.OZONE}; |
There was a problem hiding this comment.
We should enforce this and description, so the code will error out during compile.
There was a problem hiding this comment.
Good point. I removed the 'default's for tags and description.
| <parent> | ||
| <groupId>org.apache.hadoop</groupId> | ||
| <artifactId>hadoop-hdds</artifactId> | ||
| <version>0.5.0-SNAPSHOT</version> |
There was a problem hiding this comment.
is it possible to inherit this value from the parent POM ? more a question for my understanding..
There was a problem hiding this comment.
This is some kind of chicken egg problem. You should have an exact version for your parent pom as you don't know which parent should be downloaded to check the current version.
What you can remove is the normal <version> (not the <version> of the parent) which is by default the same as the parent.version (AFAIK).
But as a solution the maven version plugin is introduced to search and replace both version and parent version in a consistent way so it's not a problem.
|
|
||
| /** | ||
| * Annotation processor to generate ozone-site-generated fragments from | ||
| * ozone-site.xml. |
There was a problem hiding this comment.
wrong comment? from config classes?
| } | ||
|
|
||
| Filer filer = processingEnv.getFiler(); | ||
| System.out.println("round"); |
| if (element.getKind() == ElementKind.METHOD) { | ||
| processingEnv.getMessager() | ||
| .printMessage(Kind.WARNING, element.getSimpleName().toString()); | ||
| if (element.getSimpleName().toString().startsWith("set") |
There was a problem hiding this comment.
In future, we might want to emit a warning if you have "Set" for example, assuming that was a mistake that user is making, and letting them know we are ignoring it.
There was a problem hiding this comment.
Yes, we can implement a lot of validation. For example if you have a class with @ConfigGroup annotation but without annotated setters.
| defaultValue = "3s", | ||
| tags = {SCM, OZONE}, | ||
| description = "When a heartbeat from the data node arrives on SCM, " | ||
| + "It is queued for processing with the time stamp of when the " |
There was a problem hiding this comment.
@nandakumar131 Do we still use this Key ? or is this some code changed but we forgot to remove the config value case?
There was a problem hiding this comment.
We use it (conf.getInterval())
/**
* ReplicationMonitor thread runnable. This wakes up at configured
* interval and processes all the containers in the system.
*/
private synchronized void run() {
try {
while (running) {
final long start = Time.monotonicNow();
final Set<ContainerID> containerIds =
containerManager.getContainerIDs();
containerIds.forEach(this::processContainer);
LOG.info("Replication Monitor Thread took {} milliseconds for" +
" processing {} containers.", Time.monotonicNow() - start,
containerIds.size());
wait(conf.getInterval());
}
} catch (Throwable t) {
// When we get runtime exception, we should terminate SCM.
LOG.error("Exception in Replication Monitor Thread.", t);
ExitUtil.terminate(1, t);
}
}
I am ok with that, but some of the old school people might like a single file, and in the deployment, phase don't we need a single file ? or should we move away since the code already has the default? |
It's a very good question and I don't know the final answer. In fact we use standard hadoop Configuration features to load all the fragments, so it should be fine. I would prefer to try out this approach (with independent config fragments), but based on the feedback, experiences, we can improve/refactor it. My arguments:
<property>
<name>hdds.scm.replication.event.timeout</name>
<value>10m</value>
<final>false</final>
<source>jar:file:/opt/hadoop/share/ozone/lib/hadoop-hdds-server-scm-0.5.0-SNAPSHOT.jar!/ozone-default-generated.xml</source>
</property>It also means that we don't need to use SCM, HDDS, OZONE tags any more as they can be added based on the source. And with this approach we can print out the configuration based on the components (eg. SCM configs, common configs, etc.). Would be great to add an other information, too: which class defined the specific configuration key. |
|
💔 -1 overall
This message was automatically generated. |
|
Thank you for your comments and explanations. +1. Please feel free to commit this. Thanks for getting this done. We can now add more features into the processor, hopefully generating code for get/set and validation methods. At some point, it would be nice to have a validation method too. |
|
Thank you very much the review @anuengineer I will merge it and create follow-up jiras about validation, start to use the API at more places, generate docs, etc. This patch includes the commit of HDDS-1468 as it's based on that previous work. You already commented on that one and I addressed the comments but for the sake of the formality could you please also give me a +1 there (to make it clear that it's reviewed). |
|
👍 +1 , thanks LGTM. |
|
Thanks the review @anuengineer I am merging it right now. |
|
💔 -1 overall
This message was automatically generated. |
3 participants
See the design doc in the parent jira for more details.
In this jira I introduce a new annotation processor which can generate ozone-default.xml fragments based on the annotations which are introduced by HDDS-1468.
The ozone-default-generated.xml fragments can be used directly by the OzoneConfiguration as I added a small code to the constructor to check ALL the available ozone-default-generated.xml files and add them to the available resources.
With this approach we don't need to edit ozone-default.xml as all the configuration can be defined in java code.
As a side effect each service will see only the available configuration keys and values based on the classpath. (If the ozone-default-generated.xml file of OzoneManager is not on the classpath of the SCM, SCM doesn't see the available configs.)
See: https://issues.apache.org/jira/browse/HDDS-1469