Skip to content

Comments

Update programming guide to suggest using NewDoFn approach to paramet…#458

Closed
reuvenlax wants to merge 4 commits intoapache:asf-sitefrom
reuvenlax:new_do_fn
Closed

Update programming guide to suggest using NewDoFn approach to paramet…#458
reuvenlax wants to merge 4 commits intoapache:asf-sitefrom
reuvenlax:new_do_fn

Conversation

@reuvenlax
Copy link

@reuvenlax reuvenlax commented Jun 5, 2018

Update documentation to suggest new parameter style instead of ProcessContext.

@reuvenlax reuvenlax requested review from jkff and melap June 5, 2018 11:52
jkff
jkff approved these changes Jun 5, 2018
View reviewed changes
src/documentation/programming-guide.md Outdated
@ProcessElement
public void processElement(ProcessContext c) {
c.output(c.element().length());
public void processElement(@Element String word, OutputReceiver out) {
Copy link

@jkff jkff Jun 5, 2018

Choose a reason for hiding this comment

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

OutputReceiver<String>

Copy link
Member

@iemejia iemejia Jun 15, 2018

Choose a reason for hiding this comment

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

Is this backwards compatible to previous releases of Beam (2.x) if not, maybe we should say explictly so.

Copy link
Author

@reuvenlax reuvenlax Jun 18, 2018

Choose a reason for hiding this comment

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

@iemejia yes it is. ProcessContext still exists as before, and cannot be removed until Beam 3.0 for compatibility reasons.

Copy link
Member

@iemejia iemejia Jun 18, 2018

Choose a reason for hiding this comment

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

@reuvenlax I am not talking about ProcessContext I refer to @Element and the new annotations not included in previous versions of 2.x. Users should be aware of this given that there is not backwards compatibility on those.

src/documentation/programming-guide.md Outdated
Beam SDKs are not thread-safe*.

In addition, it's recommended that you make your function object **idempotent**.
Non-idempotent functions are supported by Beam, but xN require additional
Copy link

@jkff jkff Jun 5, 2018

Choose a reason for hiding this comment

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

xN?

src/documentation/programming-guide.md Outdated

{:.language-java}
**PipelineOptions:**
The `PipelineOptions` for the current pipeline can always be accessed in a process method by adding it as a paramet:
Copy link

@jkff jkff Jun 5, 2018

Choose a reason for hiding this comment

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

parameter

@reuvenlax
Copy link
Author

reuvenlax commented Jun 14, 2018

@melap How can I generate the docs to see what this looks like?

melap
melap reviewed Jun 15, 2018
View reviewed changes
Copy link

@melap melap left a comment

Choose a reason for hiding this comment

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

Left a couple minor comments. You can see the changes here: http://apache-beam-website-pull-requests.storage.googleapis.com/458/documentation/programming-guide/index.html (you can just use this URL and replace the PR # in it for any PR)

src/documentation/programming-guide.md Outdated
that for you. Your `@ProcessElement` method should accept an object of type
`ProcessContext`. The `ProcessContext` object gives you access to an input
element and a method for emitting an output element:
that for you. Your `@ProcessElement` method should accept parameter tagged with
Copy link

@melap melap Jun 14, 2018

Choose a reason for hiding this comment

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

parameters? or a parameter?

src/documentation/programming-guide.md Outdated
the key within the combining strategy.

##### 4.2.4.3. Combining a PCollection into a single value {#combining-pcollection}
##### 4.2. Combining a PCollection into a single value {#combining-pcollection}
Copy link

@melap melap Jun 14, 2018

Choose a reason for hiding this comment

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

was removal of the .4.3 unintentional?

src/documentation/programming-guide.md Outdated
user code might be invoked or retried; as such, keeping your function object
idempotent keeps your pipeline's output deterministic, and your transforms'
behavior more predictable and easier to debug.
effects. Non-idempotent functions are supported, however the beam model provides
Copy link

@melap melap Jun 14, 2018

Choose a reason for hiding this comment

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

Beam

src/documentation/programming-guide.md Outdated
{% github_sample /apache/beam/blob/master/sdks/python/apache_beam/examples/snippets/snippets_test.py tag:model_pardo_with_undeclared_outputs
%}```

#### 4.5.3. Accessing othe parameters in your DoFn {#other-dofn-parameters}
Copy link

@melap melap Jun 14, 2018

Choose a reason for hiding this comment

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

othe -> the

@reuvenlax
Copy link
Author

reuvenlax commented Jun 18, 2018

@iemejia added a note that prior to Beam 2.5.0 ProcessContext should still be used.

@reuvenlax
Copy link
Author

reuvenlax commented Jun 18, 2018

retest this please

10 similar comments
@reuvenlax
Copy link
Author

reuvenlax commented Jun 18, 2018

retest this please

@reuvenlax
Copy link
Author

reuvenlax commented Jun 21, 2018

retest this please

@melap
Copy link

melap commented Jun 21, 2018

retest this please

@reuvenlax
Copy link
Author

reuvenlax commented Jun 21, 2018

retest this please

@melap
Copy link

melap commented Jun 21, 2018

retest this please

@reuvenlax
Copy link
Author

reuvenlax commented Jun 22, 2018

retest this please

@reuvenlax
Copy link
Author

reuvenlax commented Jun 23, 2018

retest this please

@reuvenlax
Copy link
Author

reuvenlax commented Jun 24, 2018

retest this please

@reuvenlax
Copy link
Author

reuvenlax commented Jun 27, 2018

retest this please

@reuvenlax
Copy link
Author

reuvenlax commented Jun 27, 2018

retest this please

@kennknowles
Copy link
Member

kennknowles commented Jun 29, 2018

Noting that "Automatic staging of pull requests" is up to date while the "Jenkins: Automatic staging of pull requests" is older.

@iemejia
Copy link
Member

iemejia commented Jul 5, 2018

retest this please

@iemejia
Copy link
Member

iemejia commented Jul 5, 2018

@reuvenlax Anything missing to merge this one ? Tests are ok given Kenn coment.

@iemejia
Copy link
Member

iemejia commented Aug 20, 2018

Jumping on this one again. Anything else missing @reuvenlax / @melap ?

@reuvenlax
Copy link
Author

reuvenlax commented Aug 20, 2018 via email

@melap
Copy link

melap commented Aug 20, 2018

retest this please

@melap
Copy link

melap commented Aug 20, 2018

since the test results are over a month old, I'll retest and if all is well, will merge

@melap
Copy link

melap commented Aug 20, 2018

I fixed a couple language tab tag problems and all looks good on staging now, merging

@melap
Copy link

melap commented Aug 20, 2018

@asfgit merge

@asfgit asfgit closed this in 0ea9291 Aug 20, 2018
swegner pushed a commit to swegner/beam that referenced this pull request Sep 19, 2018
This closes apache/beam-site#458
fd3e192
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iemejia iemejia iemejia left review comments

@melap melap melap left review comments

@jkff jkff jkff approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@reuvenlax @melap @kennknowles @iemejia @jkff