Skip to content

[BEAM-4028] Transitioning MapTask objects to NameContext#5135

Merged
aaltay merged 2 commits intoapache:masterfrom
pabloem:more-namecontext
Apr 27, 2018
Merged

[BEAM-4028] Transitioning MapTask objects to NameContext#5135
aaltay merged 2 commits intoapache:masterfrom
pabloem:more-namecontext

Conversation

@pabloem
Copy link
Member

@pabloem pabloem commented Apr 16, 2018

Transitioning the operation_specs.MapTask to rely fully on NameContext for step naming.
r: @aaltay

The plan is to use NameContext wherever we need a step name (e.g. operations, ssampler), instead of passing operation_name, step_name etc around like we do atm. Classes were created in #5043, and this is further progress.

Followup will be internal changes in dataflow code to rely on and pass NameContexts around.

@pabloem
Copy link
Member Author

pabloem commented Apr 16, 2018

Run Python PostCommit

@pabloem
Copy link
Member Author

pabloem commented Apr 17, 2018

Retest this please.

@pabloem pabloem force-pushed the more-namecontext branch 3 times, most recently from 1a76a19 to 461d1d4 Compare April 19, 2018 00:14
@pabloem
Copy link
Member Author

pabloem commented Apr 19, 2018

Run Python PostCommit

Copy link
Member

@aaltay aaltay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. My only concern is that we are adding bunch of Dataflow only looking code out of dataflow runner code. We should consider finding a better home for those pieces eventually.

@pabloem
Copy link
Member Author

pabloem commented Apr 23, 2018

I understand. The goal of NameContext is to isolate naming concerns as much as possible, so that any runner (dataflow / portability) needs only to provide an instance of NameContext. Once this is pulled in to Dataflow, I expect DataflowNameContext won't be needed in the shared beam code (only in the worker code that translates from map task)

@pabloem
Copy link
Member Author

pabloem commented Apr 24, 2018

Ran seed job to update for changes from #5214

@pabloem
Copy link
Member Author

pabloem commented Apr 24, 2018

Run Seed Job

@pabloem
Copy link
Member Author

pabloem commented Apr 25, 2018

Retest this please

@pabloem
Copy link
Member Author

pabloem commented Apr 25, 2018

I believe this is ready to merge.

@aaltay aaltay merged commit 59590fe into apache:master Apr 27, 2018
@pabloem pabloem deleted the more-namecontext branch April 27, 2018 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants