Skip to content

[BEAM-3079] Update samza-runner with more features and improvements#5517

Merged
kennknowles merged 2 commits intoapache:samza-runnerfrom
xinyuiscool:samza-runner
Jun 18, 2018
Merged

[BEAM-3079] Update samza-runner with more features and improvements#5517
kennknowles merged 2 commits intoapache:samza-runnerfrom
xinyuiscool:samza-runner

Conversation

@xinyuiscool
Copy link
Contributor

Add the following feature support:

  • Stateful ParDo
  • Trigger using processing-time

Improvements:

  • Use URN to translate PTransform
  • Direct translation of Combine to avoid events buffering
  • Allow sideinput watermark to populate separately
  • Support broadcasting PCollectionView
  • Make state store point to /tmp when running the tests

@kennknowles
Copy link
Member

The reason all three precommits failed is ./gradlew :rat which checks license headers. Go ahead and fix that and I will go ahead and review.

@kennknowles kennknowles self-requested a review May 31, 2018 02:32
@xinyuiscool
Copy link
Contributor Author

Fixed the headers. Thanks!

@xinyuiscool
Copy link
Contributor Author

@kennknowles : could you please help review it when you get a chance? Thanks!

@kennknowles
Copy link
Member

Yes, sorry for the delay! Reviewing today.

@kennknowles
Copy link
Member

There's a merge commit I see from master in there. And in the diff I see UsesImpulse being added. Can you separate the upstream sync from the upgrade, or is that not possible?

@xinyuiscool
Copy link
Contributor Author

Sorry about it. Let me take a look.

@xinyuiscool
Copy link
Contributor Author

xinyuiscool commented Jun 12, 2018

@kennknowles : I squashed all my commits into one so the changes are separated from the upstream merge. I think UsesImpulses tests are added in the master and Samza doesn't support it right now.

@xinyuiscool
Copy link
Contributor Author

hmm, seems the headers are missing again in this patch. Let me quickly add them.

Copy link
Member

@kennknowles kennknowles left a comment

Choose a reason for hiding this comment

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

It is a lot to review but it pretty much LGTM. My comments are not worth blocking things on.

final TimerInternals timerInternals;
final StateInternals stateInternals;

if (signature.usesState()) {
Copy link
Member

Choose a reason for hiding this comment

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

If it doesn't use state, you don't need to create a DoFnRunnerWithKeyedInternals, do you? I think it would clean this up to refactor the constructor without the conditionals here and below.

final DoFnRunner<InputT, OutputT> doFnRunnerWithMetrics = DoFnRunnerWithMetrics
.wrap(doFnRunner, metricsContainer, stepName);

if (keyedInternals != null) {
Copy link
Member

Choose a reason for hiding this comment

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

This conditional goes right along with the one above to make just two code paths: the one where this method actually does something, and the one that is basically pass through.

outputManager,
mainOutputTag,
additionalOutputTags,
createStepContext(stateInternals, timerInternals),
Copy link
Member

Choose a reason for hiding this comment

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

Ah, is this why it is organized this way? OK.

@kennknowles kennknowles merged commit 7be38fe into apache:samza-runner Jun 18, 2018
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.

2 participants