You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by qmlmoon <gi...@git.apache.org> on 2015/02/25 18:21:51 UTC
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
GitHub user qmlmoon opened a pull request:
https://github.com/apache/flink/pull/439
[FLINK-1472] Fixed Web frontend config overview with wrong value.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/qmlmoon/incubator-flink web-conf
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/439.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #439
----
commit 2906789b6124e9f84baf05c1c555dfad9b1e2a64
Author: mingliang <qm...@gmail.com>
Date: 2015-02-25T11:27:01Z
[FLINK-1472] Fixed Web frontend config overview with wrong 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-76031299
I like the idea - that means that the webfrontend now shows what values are actually used, even if they are not in the config.
To make it even nicer, it would be cool to:
- Show which values come from the config (top) and show for which ones what default is used (in a second section below)
- add a Unit test that checks that all keys from the `ConfigConstants` are converted. I think it is easy to check this via enumerating all fields in the class using 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by qmlmoon <gi...@git.apache.org>.
Github user qmlmoon closed the pull request at:
https://github.com/apache/flink/pull/439
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-77746861
Nice, looks good to me.
+1 to merge
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-95581307
I added a Jira for this: https://issues.apache.org/jira/browse/FLINK-1936
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-95517673
Hi,
sorry for the long wait on this. I really like the feature but the implementation is not scalable: If new config values are added this needs to be updated in several places now.
Could you change ConfigConstants and add a static initializer block that builds the hash maps that you manually build in DefaultConfigKeyValues using reflection. The code would just need to loop through all fields that have _KEY at the end, and then find the matching default value without the _KEY at the end. From the default value field the type of the value can be determined and it can be added to the appropriate hash map. This way, the defaults will always stay up to date with the actual config constants.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-95561718
But then I think the solution is to normalise the constants in ConfigConstants.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-153366209
Yes, this should be addressed through a new PR.
The JIRA issue FLINK-1472 is still open.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by qmlmoon <gi...@git.apache.org>.
Github user qmlmoon commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-85207647
The default key value pairs are added manually. It seems that some of the keys in `ConfigConstant` is deleted recently.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by qmlmoon <gi...@git.apache.org>.
Github user qmlmoon commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-95523387
That would be a nice way, but
1) key/value pairs are not always distinguished by _KEY suffix, we have to format all the fields in ConfigConstants
2) some values are not in the ConfigConstants. e.g task manager slot = 1, akka_startup_time=aka_ask_timeout
In summary, we need a formatted one-to-one map of the key/value pairs in ConfigConstants
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-85170765
According to travis, the code doesn't seem to build.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by qmlmoon <gi...@git.apache.org>.
Github user qmlmoon commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-77739274
The configuration view is separated into user and default 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-95579057
I agree to enforce normalization in the `ConfigConstants`, by adding a test case that reflectively gets all fields and checks whether they start with "DEFAULT_" or end in "_KEY".
The mapping between keys/values will probably have to be done manually, unless we add annotations above the key fields declaring what the default value is.
The type of the default value should help to figure out what "get" method to use. (getInteger, getDouble) in order to get the correct conversion method.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-145854280
@qmlmoon We merged a new web dashboard a few days ago which makes this PR obsolete. Sorry for not merging it earlier. Could you close this PR please?
Thank you
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-153347995
@fhueske This issue is present in the new frontend also. See the attached screenshot:
We should probably continue in another PR though.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1472] Fixed Web frontend config overvie...
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the pull request:
https://github.com/apache/flink/pull/439#issuecomment-95580562
Yes, but then we should change this now and not build more code on top of this that can fail in the future if someone forgets to add the names to the correct hash set in some other 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---