You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by sachingoel0101 <gi...@git.apache.org> on 2015/07/29 17:16:39 UTC
[GitHub] flink pull request: [FLINK-2425]Provide access to task manager con...
GitHub user sachingoel0101 opened a pull request:
https://github.com/apache/flink/pull/952
[FLINK-2425]Provide access to task manager configuration from RuntimeEnvironment
Also fixes [FLINK-2426]: Define an UnmodifiableConfiguration class which doesn't allow modifications to the underlying configuration object.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sachingoel0101/flink flink-2426
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/952.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 #952
----
commit a8b0debbd60dd4eeb01e007e0b736f32f3724c71
Author: Sachin Goel <sa...@gmail.com>
Date: 2015-07-29T13:21:58Z
[FLINK-2425]Provide access to task manager configuration inside RuntimeEnvironment
----
---
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-2425]Provide access to task manager con...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-127144567
The earlier version had the Unmodifiable Config not as a subclass, which is crucial. Adding into the config is probably good enough, no need to over optimize this, I guess.
The assumption that mutating methods start with "set" or "add" is fair, IMHO.
---
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-2425]Provide access to task manager con...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-128252025
@StephanEwen, since this has been merged, what do you think of exposing access to task manager configuration to the runtime context? This would open up a lot of possibilities by just modifying the `TaskManagerRuntimeInfo` class for any new information we need to pass on. There's a JIRA ticket for exposing the attempt number for example. It'd fix that right away.
---
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-2425]Provide access to task manager con...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-127077747
I think that silently failing is not a good idea in this case. Silent failures always keep people wondering why things don't behave like they expect. It needs to fail with an exception here.
Also, let's make the UnmodifiableConfiguration very lightweight, so that creating it has little overhead and footprint. Best way to do this is have it actually wrap the configuration, and have all the "getX()" methods delegate to the config, while all the "setX()" methods fail with an exception.
---
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-2425]Provide access to task manager con...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-128252301
It should however be called `TaskRuntimeInfo` in such a case, since it is being passed on to every task individually anyway.
---
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-2425]Provide access to task manager con...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-127184585
Looks good, modulo some styles...
- No need to override all "set*" methods, when the internal set method is overridden.
- Final modifiers should be used for new members where possible, consistent with the remaining code
- Let's not add new Scala classes. We decided that in the long run, it would be good to have the runtime pure Java again.
I need this quite soon, so let me take this over...
---
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-2425]Provide access to task manager con...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-127185418
Ah. Okay. You can fix the remaining things.
This build also fails on the StreamCheckpointingITCase. I'll file another JIRA ticket for that. I've observed that twice.
---
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-2425]Provide access to task manager con...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-127071830
Added test to verify all setter methods are overridden by the `UnmodifiableConfiguration` 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.
---
[GitHub] flink pull request: [FLINK-2425]Provide access to task manager con...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-127077814
For the test it is important that it catches the case where someone adds a method (for example "setStringArray()") to the base class and does not override it in the superclass.
We could also catch that easier, if we make the "setInternal()" method package private and override it in the UnmodifiableConfiguration to throw an exception.
---
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-2425]Provide access to task manager con...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-127053317
If we want to unmodifiable configuration to be usable like any other configuration, it needs to be a subclass that overrides all mutating methods.
To make sure that is safe, it should also have a test that sees that all "set*" methods from the superclass are overridden.
---
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-2425]Provide access to task manager con...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 closed the pull request at:
https://github.com/apache/flink/pull/952
---
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-2425]Provide access to task manager con...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-128670330
@sachingoel0101 Since this has been merged, could you close the PR?
---
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-2425]Provide access to task manager con...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/952#issuecomment-127139227
> ...have it actually wrap the configuration, and have all the "getX()" methods delegate to the config,
> while all the "setX()" methods fail with an exception.
That was my earlier solution [which is gone now because I rebased and force pushed :( ]. You're right though. It'd certainly make the initialization easier, but adds an additional calling overhead for every getter method [which are the most frequent. Initialization will be done exactly once]. IMO that's not a good idea.
>I think that silently failing is not a good idea in this case. Silent failures always keep people >wondering why things don't behave like they expect. It needs to fail with an exception here.
That's easy enough! :') I just wasn't sure.
>For the test it is important that it catches the case where someone adds a method (for example >"setStringArray()") to the base class and does not override it in the superclass.
The assumption being that any modifiers will be named `set*`?
---
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.
---