[FLINK-2488] Expose Attempt Number in RuntimeContext

[sachingoel0101]

Passes the attempt number all the way from TaskDeploymentDescriptor to the RuntimeContext.
Small thing I want to confirm: For RuntimeContext in Tez, is it okay to use TaskContext#getTaskAttemptNumber provided by Tez as a proxy for the attempt number?

[rmetzger]

Can you maybe extend a test which is restarting a job multiple times and check there that the attempt number is set properly?

[sachingoel0101]

@rmetzger I have added a test case to verify the functionality.

[sachingoel0101]

Hi @rmetzger, can you take another look? I need to base some further work on this [related to cleaning up passing of task name, index, etc.].

[rmetzger]

Hey @sachingoel0101, I think @StephanEwen is the best contact person here. Since this affects a very important interface we shouldn't make any mistakes ;)

[sachingoel0101]

I'll wait for Stephan to review this then.

[StephanEwen]

I'll shepherd this pull request in...

[StephanEwen]

This generally looks good and pretty straight forward.
As such it is actually good to merge.

I remember that a while back, we were discussing to create a TaskInfo object that would contain "subtaskIndex", "parallelism", "name", "name-with-subtask", "attempt", "vertex id", "attempt id", etc...
Having such an object would allow to pass all these elements simply from the TaskDeploymentDescriptor to the Task to the RuntimeContext. And whenever we add another field, we need not propagate it manually through all the functions calls, but simply add it to the TaskInfo.

What do you think?

[sachingoel0101]

Yes. I was planning to start working on that after this. Since it's
preferable to fix different jiras in different commits, it'd be good if I
can base my work after this is committed. What do you think?
On Dec 2, 2015 6:39 PM, "Stephan Ewen" notifications@github.com wrote:

This generally looks good and pretty straight forward.
As such it is actually good to merge.

I remember that a while back, we were discussing to create a TaskInfo
object that would contain "subtaskIndex", "parallelism", "name",
"name-with-subtask", "attempt", "vertex id", "attempt id", etc...
Having such an object would allow to pass all these elements simply from
the TaskDeploymentDescriptor to the Task to the RuntimeContext. And
whenever we add another field, we need not propagate it manually through
all the functions calls, but simply add it to the TaskInfo.

What do you think?

—
Reply to this email directly or view it on GitHub
#1386 (comment).

[StephanEwen]

As far as I can see from these changes, introducing a TaskInfo will completely subsume this change here and change every line again.

You are right, usually different issues should go into different pull requests. But here, one change is a superset of the other change, so that the "task attempt" change without the "task info" change becomes obsolete.

I am not strictly against merging this, just thinking that it makes sense to put the TaskInfo into this change as well...

[sachingoel0101]

@StephanEwen you're absolutely right. It will most certainly do that.
I will push both things in a single commit and file an additional jira for introducing the TaskInfo object.

[sachingoel0101]

@StephanEwen can you take a look again? I have introduced the TaskInfo object in the second commit, and we can squash them before merging.

[StephanEwen]

Checking this out...

[StephanEwen]

I am adding a number of comments. When you address them, please do not squash commits, so we can review just the changes separately.

[StephanEwen]

Good set of checks. Would be good to add a non-null check for taskName as well.
Optional: statically importing ´checkArgument` makes it even nicer.

[StephanEwen]

All in all, pretty good change. Makes many parts of the code nicer. Also good java docs and comments!

Comments were added inline. Nothing major, few details.

[sachingoel0101]

@StephanEwen thanks for the review. I have addressed all but one of your comments.

[sachingoel0101]

Travis passes. Should be good to merge.
This also resolves FLINK-2524, so the commit message should reflect that too.

[StephanEwen]

Thanks for addressing the comments.

+1, will merge this...