[FLINK-2458][FLINK-2449]Access distributed cache entries for CollectionExecution and in Iterative tasks. #970

sachingoel0101 · 2015-08-01T16:02:43Z

This PR adds support for accessing distributed cache entries when running iterations.
Since there are several tests which execute on both Cluster and Collection modes, it seems logical to not fail a test on either if it passes on both. Distributed Cache files create one such case. There is nothing actually wrong with trying to access a distributed cache entry when running in collection environment. It just doesn't really make sense to do so.
This takes care of that too.

zentol · 2015-08-07T12:03:19Z

...core/src/main/java/org/apache/flink/api/common/functions/util/AbstractRuntimeUDFContext.java

@@ -79,7 +68,7 @@ public AbstractRuntimeUDFContext(String name,
 		this.subtaskIndex = subtaskIndex;
 		this.userCodeClassLoader = userCodeClassLoader;
 		this.executionConfig = executionConfig;
-		this.distributedCache = new DistributedCache(cpTasks);
+		this.distributedCache = Preconditions.checkNotNull(new DistributedCache(cpTasks));


Don't you want to check cpTasks for being null?

Ah yes. Sorry.

sachingoel0101 · 2015-08-07T15:25:02Z

Addressed PR comments. There is one unrelated failure on the GroupReduceITCase. I've filed a JIRA for that.

StephanEwen · 2015-08-10T14:55:56Z

Looks good, in general.

Can you add the test to one of the other iteration test files? This saves cluster startup and shutdown costs, making builds faster. Maybe to the iteration aggregators, or iteration accumulators.

StephanEwen · 2015-08-10T14:56:17Z

flink-core/src/main/java/org/apache/flink/api/common/operators/CollectionExecutor.java

@@ -501,4 +536,22 @@ public int getSuperstepNumber() {
 			return (T) previousAggregates.get(name);
 		}
 	}
+
+	private static final class DoingNothing implements Callable<Path>{


It actually does something ;-)

Haha. Yes. In an earlier version of the code, it wasn't. :')

sachingoel0101 · 2015-08-10T23:44:43Z

I've moved the test to an existing MultipleProgramTestBase. Should be good to merge now. :)

sachingoel0101 · 2015-08-16T07:17:59Z

I'd like to get this merged soon. This removes multiple constructors for Runtime contexts and establishes a clean hierarchy, making any changes to the constructors easier. This will be useful for two Jiras on exposing task configuration and task attempt number to the Runtime context.

sachingoel0101 · 2015-08-16T08:56:04Z

These changes have been reverted back
I decided to go ahead and implement things which touch the Runtime Context constructors with this PR. This now closes five Jiras, namely 2449, 2458, 2488, 2496 and 2524. Commit messages are descriptive of each Jira.
Flink-2449: Allow access to distributed cache from Collection Environment
Flink-2458: Allow access to distributed cache from Iterative Tasks
Flink-2488: Expose Attempt number from Runtime Context
Flink-2496: Expose Task Manager configuration in Runtime Context
Flink-2524: Add getTaskNameWithSubtasks in Runtime Context.

hsaputra · 2015-08-16T09:11:52Z

flink-runtime/src/main/scala/org/apache/flink/runtime/taskmanager/TaskManager.scala

@@ -897,7 +897,7 @@ class TaskManager(
        config.timeout,
        libCache,
        fileCache,
-        runtimeInfo)
+        new TaskRuntimeInfo(hostname, taskManagerConfig, tdd.getAttemptNumber))


Why is this changed from before?

This is to provide access to Task attempt number from Runtime Context. I should add a description of the other tickets this resolves.
Is this a good idea though? To fix five issues in one PR? Or should I open a separate one and keep this one for just distributed cache?

generally we try to keep one PR for one issue, exceptions should only be done for closely related issues.

why did you decide to add these issues into this PR? ( i have a hard time understanding it, since the commits barely touch the same files.

Yes. The addition of distributed cache removes the need for multiple constructors for RuntimeContexts. Since providing access to runtime information needed changing the constructors, I deemed it better to work with what would be the only needed constructors after merging this.
I can revert this commit and open a separate PR for the other three issues if necessary.

I would prefer if you opened a second PR once this is merged. The issues are not really related to each other; the 2nd commit was simply made based on the 1st commit. We would end up having two separate discussions in 1 PR, which i think is a bad idea.

Ah. Yes. That makes sense. I will revert this and open a separate PR. Apologies.

sachingoel0101 · 2015-08-16T10:05:21Z

Reverting back to make this PR only about the distributed cache.

StephanEwen · 2015-08-16T15:04:52Z

We are indeed falling behind on merging pull requests, right now. Many committers are on vacation this month, and for the others, the large amount of pull requests is hard to keep up with, especially next to the work on our own issues.

Hope this will get better in a week or two.

I'll try to get a look at this very soon...

StephanEwen · 2015-08-16T15:10:27Z

In the CollectionExecutor, can you skip creating the ExecutiorService? You can eagerly resolve the path and then put an already finished future into the map.

StephanEwen · 2015-08-16T15:12:12Z

Aside from the comment above, this looks good. Would merge this, after the comment is addressed.

[FLINK-2449]Allow use of distributed cache from Collection Environments

sachingoel0101 · 2015-08-16T15:38:01Z

Addressed comments. @StephanEwen

StephanEwen · 2015-08-16T16:33:03Z

Looks good, merging this!

…from Iteration contexts & use of distributed cache from Collection Environments This closes apache#970

sachingoel0101 force-pushed the iteration_cache_files branch 2 times, most recently from 1a1ddb3 to 0675cb4 Compare August 6, 2015 20:18

zentol reviewed Aug 7, 2015
View reviewed changes

sachingoel0101 force-pushed the iteration_cache_files branch 2 times, most recently from c22cdec to 05c5326 Compare August 7, 2015 14:07

sachingoel0101 force-pushed the iteration_cache_files branch from 05c5326 to e571f3b Compare August 7, 2015 17:53

StephanEwen reviewed Aug 10, 2015
View reviewed changes

sachingoel0101 force-pushed the iteration_cache_files branch 3 times, most recently from fe9bb3a to 376425c Compare August 10, 2015 22:17

sachingoel0101 force-pushed the iteration_cache_files branch from 376425c to a8d1385 Compare August 16, 2015 06:51

sachingoel0101 force-pushed the iteration_cache_files branch from 885fdd2 to a1a3824 Compare August 16, 2015 08:53

hsaputra reviewed Aug 16, 2015
View reviewed changes

sachingoel0101 force-pushed the iteration_cache_files branch from a1a3824 to a8d1385 Compare August 16, 2015 10:03

sachingoel0101 mentioned this pull request Aug 16, 2015

[FLINK-2488][FLINK-2496] Expose Task Manager configuration and Task attempt number to Runtime context #1026

Closed

sachingoel0101 force-pushed the iteration_cache_files branch from a8d1385 to a37b329 Compare August 16, 2015 15:34

[FLINK-2458]Access distributed cache entries from Iteration contexts.

e264224

[FLINK-2449]Allow use of distributed cache from Collection Environments

sachingoel0101 force-pushed the iteration_cache_files branch from a37b329 to e264224 Compare August 16, 2015 15:36

asfgit closed this in 358259d Aug 16, 2015

sachingoel0101 deleted the iteration_cache_files branch August 23, 2015 14:56

nikste pushed a commit to nikste/flink that referenced this pull request Sep 29, 2015

[FLINK-2458] [FLINK-2449] [runtime] Access distributed cache entries …

6acf46c

…from Iteration contexts & use of distributed cache from Collection Environments This closes apache#970

rmetzger added the component=Tests label Mar 14, 2019

[FLINK-2458][FLINK-2449]Access distributed cache entries for CollectionExecution and in Iterative tasks. #970

[FLINK-2458][FLINK-2449]Access distributed cache entries for CollectionExecution and in Iterative tasks. #970

Uh oh!

Conversation

sachingoel0101 commented Aug 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachingoel0101 commented Aug 7, 2015

Uh oh!

StephanEwen commented Aug 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachingoel0101 commented Aug 10, 2015

Uh oh!

sachingoel0101 commented Aug 16, 2015

Uh oh!

sachingoel0101 commented Aug 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachingoel0101 commented Aug 16, 2015

Uh oh!

StephanEwen commented Aug 16, 2015

Uh oh!

StephanEwen commented Aug 16, 2015

Uh oh!

StephanEwen commented Aug 16, 2015

Uh oh!

sachingoel0101 commented Aug 16, 2015

Uh oh!

StephanEwen commented Aug 16, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants