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/08/16 12:14:46 UTC
[GitHub] flink pull request: [FLINK-2488][FLINK-2496] Expose Task Manager c...
GitHub user sachingoel0101 opened a pull request:
https://github.com/apache/flink/pull/1026
[FLINK-2488][FLINK-2496] Expose Task Manager configuration and Task attempt number to Runtime context
This PR fixes these issues:
1. [FLINK-2496]Expose Task Manager configuration to Runtime Context
2. [FLINK-2488]Expose attempt number of task to Runtime Context
3. [FLINK-2524]Add getTaskNameWithSubtasks to Runtime Context
Depends on #970 for simplistic constructors for `RuntimeContext`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sachingoel0101/flink flink-2488
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1026.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 #1026
----
commit a8d138519bfcf24fb63028b6ecae3e384a05b69d
Author: Sachin Goel <sa...@gmail.com>
Date: 2015-08-01T13:55:42Z
[FLINK-2458]Access distributed cache entries from Iteration contexts.
[FLINK-2449]Allow use of distributed cache from Collection Environments
commit 5fbdebe5742b26683a2755d80681cc505962fc02
Author: Sachin Goel <sa...@gmail.com>
Date: 2015-08-16T07:44:39Z
[FLINK-2496]Expose Task Manager configuration to Runtime Context
[FLINK-2488]Expose attempt number of task to Runtime Context
[FLINK-2524]Add getTaskNameWithSubtasks to Runtime Context
----
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-137009547
Rebased to the current master. This should be mergeable now. Travis fails on unrelated kafka and flink-fs-tests. Just re-triggered another build.
@StephanEwen
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-138404446
Will there be any further review of this?
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/1026#discussion_r37294536
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/TaskRuntimeInfo.java ---
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.api.common;
+
+/**
+ * Encapsulation of runtime information for a Task, including Task Name, sub-task index, etc.
+ *
+ */
+public class TaskRuntimeInfo implements java.io.Serializable {
+
+ /** Task Name */
+ private final String taskName;
+
+ /** Index of this task in the parallel task group. Goes from 0 to {@link #numParallelTasks} - 1 */
+ private final int subTaskIndex;
+
+ /** Number of parallel running instances of this task */
+ private final int numParallelTasks;
+
+ /** Attempt number of this task */
+ private final int attemptNumber;
+
+ /** Task Name along with information on index of task in group */
+ private transient final String taskNameWithSubTaskIndex;
--- End diff --
If this is transient, it is null after deserialization. Either make it non-transient, or re-compute it after deserialization.
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-131771732
Yes, let's have a `TaskRuntimeInfo` and a `TaskManagerContext`. Both are available in the `RuntimeEnvironment` and passed to the `RuntimeContext`.
The `TaskRuntimeInfo` has all task-specific information, the `TaskManagerContext` all cross-task constant parts. It could also hold the I/O manager, memory manager, ...
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-141653638
Unrelated failure on travis. Filed a jira [2711]
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-132198739
Rebased to the latest master. Can be merged 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-2488][FLINK-2496] Expose Task Manager c...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-131767636
Looks mostly good.
The `TaskRuntimeInfo` is now a bit strange. It contains some runtime info for tasks (attempt number) but not all (subtasks, etc). The attempt number is in the `TaskDeploymentDescriptor` anyways, so why copy it in addition to the `TaskRuntimeInfo`? Before, the `TaskManagerInfo` was clearly the context info of the TaskManager that was the same for all tasks and there was no duplicate information.
Looks like the motivation was to minimize the number of objects passed to the `RuntimeContext`. In that case, why not create a `RuntimeInfo` for the task (keep the `TaskManagerInfo`), put all the task-specific information in there, pass it to the `RuntimeEnvironment` and `RuntimeContext` and let them return all info like `getTaskName` and `getIndexOfThisSubtask` from there?
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on a diff in the pull request:
https://github.com/apache/flink/pull/1026#discussion_r37173958
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/deployment/TaskDeploymentDescriptor.java ---
@@ -120,7 +125,7 @@ public TaskDeploymentDescriptor(
List<InputGateDeploymentDescriptor> inputGates,
List<BlobKey> requiredJarFiles, int targetSlotNumber) {
- this(jobID, vertexID, executionId, taskName, indexInSubtaskGroup, numberOfSubtasks,
+ this(jobID, vertexID, executionId, taskName, indexInSubtaskGroup, numberOfSubtasks, 0,
--- End diff --
This constructor was only being used in test classes, and the attempt number would've been kept 1 for them all.
But yes. You're right. Leads to more consistency. Will fix this 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2488][FLINK-2496] Expose Task Manager c...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/1026#discussion_r37173778
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/deployment/TaskDeploymentDescriptor.java ---
@@ -120,7 +125,7 @@ public TaskDeploymentDescriptor(
List<InputGateDeploymentDescriptor> inputGates,
List<BlobKey> requiredJarFiles, int targetSlotNumber) {
- this(jobID, vertexID, executionId, taskName, indexInSubtaskGroup, numberOfSubtasks,
+ this(jobID, vertexID, executionId, taskName, indexInSubtaskGroup, numberOfSubtasks, 0,
--- End diff --
I think such constructors with default values are dangerous, as people tend to call the wrong constructors.
Someone who creates the deployment descriptor should think about providing the attempt number.
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-131768999
Yes. Minimizing the arguments being passed to `RuntimeContext` was the motivation. I thought about putting every task specific field into the TaskInfo object but since it isn't a real problem, hesitated.
Should I do that then? I certainly like the idea. The constructors for `RuntimeContext` are somewhat messy, and doing this would make any future changes to contexts a lot easier.
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-132199820
Fixed.
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-131853677
I have implemented a `TaskRuntimeInfo` which holds descriptive information about the task, such as name, index, parallelism, etc. Also renamed the `TaskManagerRuntimeInfo` to `TaskManagerContext` and included the `IOManager`, `MemoryManager` and `NetworkEnvironment`.
The `RuntimeContext`s now use these two fields in their constructor.
Further, for minimizing number of changes while modifying the `RuntimeContext`s in future, I have also modified the test classes to use a *create runtime* utility function.
Waiting for travis to give the green light for 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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-131660699
Since #970 is in, this can be merged 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-2488][FLINK-2496] Expose Task Manager c...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-132322001
Sorry to be picky again, but it would be good to harmonize the code style a bit - make it more consistent with the remaining code.
What struck me in particular the omission of space before the curly braces at the start of methods and other code blocks. All other parts of the code have that.
There are ongoing discussions about making the code style stricter (also with respect to white spaces), and this is much easier if new code follows this standard already. Otherwise, there'd going to be a lot of style adjustments necessary when introducing the check.
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-138490882
Yeah, I'd like to add this once I find the time. Bit overloaded right now, please bear with us for a bit...
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-137116109
Travis Passes successfully. https://travis-ci.org/apache/flink/builds/78368635
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-132449408
Sure thing. I'll fix it. IntelliJ re-formats used to take care of such things, but I've stopped using it recently as it was messing up scala code.
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-157818292
Yes. That's why I closed it. :')
I'm thinking of having a construct named `TaskInfo`, which contains information like name, index, parallel tasks, attempt number, etc. which will be passed all the way down from the `TDD` to the `RuntimeContext`. Let me know if that's a good 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2488][FLINK-2496] Expose Task Manager c...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-157824468
Yes, a TaskInfo is a good idea.
It can be handed also to the `RuntimeEnvironment` (from there to the `RuntimeContext`), and is created once in the Task constructor from the TDD (that we we spare the JobManager from changes).
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 closed the pull request at:
https://github.com/apache/flink/pull/1026
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-157826454
Agreed.
I also would like to cleanup the several getter functions for these fields. We can just add a `getTaskInfo` in the `TDD`, `Task` and `Environment`. The fields can then be accessed from this object.
Since this isn't the user-facing API, it should be fine IMO. `RuntimeContext` will still provide separate access to every field.
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on a diff in the pull request:
https://github.com/apache/flink/pull/1026#discussion_r37173822
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/functions/util/AbstractRuntimeUDFContext.java ---
@@ -93,6 +93,15 @@ public int getIndexOfThisSubtask() {
}
@Override
+ public String getTaskNameWithSubtasks() {
--- End diff --
Will fix this.
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-138503010
Sure. No worries. :)
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-132453057
@StephanEwen , I have fixed the styling issues as much as I could find out.
Thanks for your patience. :)
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/1026#discussion_r37173574
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/functions/util/AbstractRuntimeUDFContext.java ---
@@ -93,6 +93,15 @@ public int getIndexOfThisSubtask() {
}
@Override
+ public String getTaskNameWithSubtasks() {
--- End diff --
Since this method may be called rather often, I would create this string once and return it then.
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-157753581
Sorry for dropping this. The feature is still interesting. I have the feeling it can probably be implemented a bit simpler...
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-155921293
Closing this for now. Needs reworking.
---
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-2488][FLINK-2496] Expose Task Manager c...
Posted by sachingoel0101 <gi...@git.apache.org>.
Github user sachingoel0101 commented on the pull request:
https://github.com/apache/flink/pull/1026#issuecomment-141461094
Is it possible to get this in soon? I need access to the task manager configuration for something I'm working on. @StephanEwen
---
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.
---