[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3495
i will make a number of changes before merging it.
- added a hack to deal with options using
`System.getProperty("java.io.tmpdir")` as the default value
- replace `>` and `<` in descriptions and default values
- all keys are now tagged as `` to allow linking
- empty String default values are treated as not having a default value
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 I think we're good to go now; will try it out again after a rebase and merge it unless something major comes up. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 I would use the class name, as otherwise we add an additional source of mistakes. When introducing the `ConfigGroups` annotation the first time; if the configured default name doesn't match the class name the docs will break if not adjusted properly. In my mind when you want to extract some options into a separate table for the docs you should only have to worry about exactly that, and not about breaking the docs for the rest of the class. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/3495
It works exactly as you describe. The role of `defaultGroup` in
`ConfigGroups` is setting a name of the defaultGroup. I agree using
`ConfigGroup` for the `defaultGroup` may be counter intuitive.
What do you think of sth like:
```
@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
@Internal
public @interface ConfigGroups {
String defaultGroupName();
ConfigGroup[] additionalGroups() default {};
}
```
or would you prefer removing it completely and just using the name of the
enclosing class?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 Couldn't we create the default group implicitly by taking all options that aren't part of any ConfigGroup? As in having ConfigGroup only describing sub-groups that should be excluded from the default group? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/3495 I do like you code, only thing missing I had in mind yesterday was an idea of defaultGroup, which is an easy way to group options with not overlapping prefixes e.g. right now in documentation (in doc files) of jobmanager we have options like jobmanager.* and blob.*. I modified a little bit your solution to enable the defaultGroup. What do you think of that idea? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 I love this issue, it's just so damn interesting. You can find an implementation of my approach [here](https://github.com/zentol/flink/commit/2c40e660a883ec4db28cf92913daf0a103563fc8). First we generate a tree structure containing all ConfigOptions, based on the components of the key. We then insert the ConfigGroups into the corresponding nodes, marking sub trees. Finally, for each ConfigGroup we recursively search the entire tree, extracting all ConfigOptions that are contained in the sub tree of the ConfigGroup, that are not part of another, more specific, ConfigGroup. It also as a separate branch for *Options classes that don't have any annotation, making them optional. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 If we allow supplying a regex instead we could separate "jobmanager" and "jobmanager.web". Or we could could make the matching exclusive and start with the most specific groups first. I think we can solve this without explicitly assigning ConfigOptions to groups. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/3495
How about adding another class like ConfigGroup?
```
public static final ConfigGroup WEB_FRONTEND = new
ConfigGroup("WebFrontend",
WEB_BACKPRESSURE_DELAY,
WEB_BACKPRESSURE_NUM_SAMPLES
...
);
```
Unfortunately then we would need to add every option to a group. We could
add a testCase that will check if every option belongs to some ConfigGroup.
Also we could provide a helper annotation `@ConfigGroup(String name)` for
classes that all options belong to single ConfigGroup.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/3495 There is a problem with the prefix solution. See the mentioned JobManager and WebFrontentd options, prefix for JobManager options is "jobmanager" which also covers prefix for WebFrontend options which is "jobmanager.web". --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3495
2 more issues popped up:
A large number of configuration parameters have not been ported yet (see
[FLINK-4765](https://issues.apache.org/jira/browse/FLINK-4765) which means that
we cannot use this for all options right away.
More importantly, our organization of ConfigOptions isn't coherent with the
existing organization in the docs. For example, all JobManager and WebFrontend
options life in `JobManagerOptions`, but in the docs they are 2 different
tables. The same issue applies to the Security Options, with SSl, Kerberos and
Zookeper sharing the same file.
Creating the tables based on the ConfigOption classes does not appear to be
granular enough to provide a good documentation.
I'm currently leaning towards adding annotations that group the contained
config options based on a key prefix. Here's an example how it could look like
for the `SecurityOptions`:
```
@ConfigGroups(
@ConfigGroup("Kerberos", "security.kerberos"),
@ConfigGroup("SSL, "security.ssl"),
@ConfigGroup("Zookeeper", "zookeeper"))
public class SecurityOptions {
}
```
In this example we would use this annotation to generate 3 different
tables, with proper names.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 I'm also thinking that we should hide ConfigOptions that are marked as deprecated, which should be rather easy to fix since we're already employing reflection. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 These options are affected by the above: ```JobManagerOptions#WEB_TMP_DIR``` ```PythonOptions#DC_TMP_DIR``` The ```HistoryServerOptions#HISTORY_WEB_DIR``` does not define a default value, which isn't really accurate since it will generate a default value like the above, just not in the `ConfigOption`. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3495
Ran into an issue. Some config parameters, like `jobmanager.web.tmpdir`,
have non-constant default values that are evaluated at runtime, like
`System.getProperty("java.tmpdir")`. These values now show up in the generated
table as the default value.
I see two ways to fix this: Either enforce, manually i might add, that all
default values must be constant, or explicitly mark these kind of default
values as dynamic in the OptionBuilder, with a new method like
`withDynamicDefaultValue(T value, String origin)`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 I'll check this out again today. I will try to integrate the generated tables into the respective documentation to see if there isn't some edge case we haven't considered. If I don't find anything i will merge it and open a separate PR with the docs integration. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user uce commented on the issue: https://github.com/apache/flink/pull/3495 I think it's up to you guys whether you want the short description in there or not. I see that it only makes sense if you actually invest the time to have a good short description. You would probably need a table like this then: ``` +-+-++ | key | default | short description | +-+-++ | long description with details (expandable) | ++ ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 Also, let's drop the short descriptions. If we ever feel the need to have them we can always add them again. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 We should also update the docs for building the docs (`docs/README.md`) with instructions on how to regenerate the tables. A small note on how to integrate these tables into the docs would be sweet too. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 Wouldn't be surprised if @uce could provide some useful input regarding the short description. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/3495 I will address the issues with formatting default value tomorrow morning. As for the `shortDescription` it came out on the initiative of ML folks (at least it is written as such in [FLINK-5780](https://issues.apache.org/jira/browse/FLINK-5780)). If there is no longer a valid reason for it I would also be for dropping the shortDescription. Unfortunately I am not sure who from ML should we ask about it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 I'm also not sure about including the short description. Looking at the existing documentation descriptions this would result in pretty large table entries with a lot of whitespace in the short description column, and probably an inherent redundancy. We can better solve this by writing the large descriptions as such that they have a succinct first line, followed by further details; basically as we do right now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 This is working pretty nicely. Just tried out appending the generated table to the SSL documentation, here's the result:  That said, the default value column could use some improvements. Options without a default value should be explicitly marked as such (like a dash or smth), and Strings should be displayed with quotes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 That's a neat idea to re-use them. Let's make this part of the PR though. You aren't limited to flink-core, you just have to put the plugin into other modules as well. You would have to to the same with your original approach, unless you're suggesting to build the docs for flink-mesos in flink-core which is just asking for trouble. Adjusting the code from me to allow a different regex/search path is easy to do. Except the MesosTaskManagerParameters all existing usages match already. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/3495
Ad second comment: My intention was to include the generated tables into
markdown using command like:
`{% include generated/high_availability_configuration.html %}`
This way it is possible to include the config options in different files,
not jus in docs/setup.config.md
Ad first comment: I think there are couple of problems with the approach of
finding the files by Regex.
1. You are limited to just one module. Right know `ConfigOption` is used
in: flink-core, flink-runtime, flink-mesos, flink-yarn
2. You are limited with choosing the file name. (this one is not so
important)
Just as a side note, I tried to follow the solution used in Apache Kafka.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 This feature only really makes sense if we can integrate it directly into the web documentation. This should actually be rather simple easy. 1) Create a copy of the existing `/docs/setup/config.md` that includes a placeholder. 2) Instead of generating several html pages, instead generate a single markdown table modeled like [this](https://ci.apache.org/projects/flink/flink-docs-master/monitoring/metrics.html#system-metrics) 3) In the antrun task, generate this table, copy the modified config.md to `/docs/setup/config.md` while replacing the placeholder with the generated markdown table. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/3495
Alright here's what i came up with. We define a single antrun task which
does the following:
Since all *Options classes reside in the same directory we just take a look
in there and filter out the Options classes. We then load the class via
reflection, generate the html and manually write out the file.
The antrun plugin in flink-core is configured like this:
```
maven-antrun-plugin
1.7
high-availability
package
run
```
While the ConfigGroup has gotten a new main method:
``´
public static void main(String[] args) throws Exception {
String basePath = args[0];
File path = new
File("../src/main/java/org/apache/flink/configuration/");
String[] classes = path.list();
Pattern p2 = Pattern.compile("([A-Z][a-z]*)");
Pattern p = Pattern.compile("([a-zA-Z]*Options).java");
for (String clazz : classes) {
Matcher m = p.matcher(clazz);
if (m.matches()) {
String className = m.group(1);
if (!className.equals("ConfigOptions")) {
Matcher m2 = p2.matcher(clazz);
List parts = new ArrayList<>();
while (m2.find()) {
parts.add(m2.group(1));
}
StringBuilder sb = new StringBuilder();
for (int x = 0; x < parts.size() - 1; x++) {
sb.append(parts.get(x).toLowerCase());
sb.append("_");
}
sb.append("configuration.html");
String generatedDocFilePath = basePath + sb;
String fullClassPath =
"org.apache.flink.configuration." + className;
String doc =
create(Class.forName(fullClassPath)).toHTMLTable(false);
File f = new File(generatedDocFilePath);
f.getParentFile().mkdirs();
f.createNewFile();
try (FileWriter fw = new FileWriter(f)) {
fw.write(doc);
fw.flush();
}
}
}
}
}
```
Haven't cleaned the code up yet, since it's 11pm after all, but it works.
Maybe you can refine it a bit.
The result is that the docs will be generated automatically for every new
*Options class that is placed under org.apache.flink.configuration in
flink-core, which is a must-have imo.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3495 I'm playing around with this right now. Seems like it works; don't know though if the resulting html is what people expected; since you know, it is pretty bare-bones. :) Anyway, I'm looking into ways so we don't have to throw a main method into every Options class. Would be even more rad if you just detect them with reflection and wouldn't need multiple executions in the pom that have to be updated all the time. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3495: [FLINK-5781][config] Generation HTML from ConfigOption
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/3495 Could someone have a look on it? I saw other modules start to migrate to `ConfigOption` so I think would be nice to use it for docs generation. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
