Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-8865] Updating Javadoc of FileIO example #10256

Merged
merged 1 commit into from Dec 16, 2019

Conversation

@suztomo
Copy link
Contributor

suztomo commented Dec 2, 2019

https://issues.apache.org/jira/browse/BEAM-8865

The existing Javadoc of FileIO has two problems:

  • Function KVs does not exist. TypeDescriptors has kvs function.
  • readFullyAsUTF8String throws IOException and thus cannot be used in stream.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

* return KV.of(
* f.getMetadata().resourceId().toString(), f.readFullyAsUTF8String());
* } catch (IOException ex) {
* throw new RuntimeException("Failed to read the file", ex);

This comment has been minimized.

Copy link
@suztomo

suztomo Dec 2, 2019

Author Contributor

Is there a better idea than this exception handling?

This comment has been minimized.

Copy link
@jkff

jkff Dec 3, 2019

Contributor

via() takes a Contextful<Fn<..>>, where Fn.apply() is allowed to throw Exception (https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Contextful.java#L70) - so the original snippet should compile, no?

This comment has been minimized.

Copy link
@suztomo

suztomo Dec 3, 2019

Author Contributor

Interesting; is there a way to use lambda while letting a checked exception thrown?

My IntelliJ complains as below:

Screen Shot 2019-12-03 at 09 07 42

Also from Maven:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.7.0:compile (default-compile) on project word-count-beam: Compilation failure
[ERROR] /Users/suztomo/word-count-beam/src/main/java/org/apache/beam/examples/WordCount.java:[225,85] unreported exception java.io.IOException; must be caught or declared to be thrown

IntelliJ says it picks up this method (not the one with Contextful):

  /** Binary compatibility adapter for {@link #via(ProcessFunction)}. */
  public <NewInputT> MapElements<NewInputT, OutputT> via(
      SerializableFunction<NewInputT, OutputT> fn) {
    return via((ProcessFunction<NewInputT, OutputT>) fn);
  }

Am I using the example in a wrong way?

This comment has been minimized.

Copy link
@jkff

jkff Dec 3, 2019

Contributor

Ah, then maybe the example should add Requirements.empty() to force this overload? https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Contextful.java#L122 There's probably some frustrating Java reason why we can't just add an fn(Fn) overload - Java probably wouldn't be able to disambiguate between it and the other overloads when you supply a lambda.

This comment has been minimized.

Copy link
@suztomo

suztomo Dec 3, 2019

Author Contributor

IntelliJ says: Cannot resolve method 'via(<lambda expression>, org.apache.beam.sdk.transforms.Requirements)'

image

The below worked. Is this what you're looking for?:

        .apply(MapElements.into(kvs(strings(), strings()))
            .via(Contextful.fn(
                (ReadableFile f, Context c) -> KV.of(f.getMetadata().resourceId().toString(), f.readFullyAsUTF8String()),
                Requirements.empty()
            )))

image

This comment has been minimized.

Copy link
@suztomo

suztomo Dec 10, 2019

Author Contributor

@jkff Did you get a chance to check the code above?

This comment has been minimized.

Copy link
@jkff

jkff Dec 10, 2019

Contributor

Sorry I missed this. Hmm, this is getting ugly enough that I think the original code you proposed is better - it's, at least, idiomatic in Java. Would be nice (not in this PR) to make it easier to do MapElements.via() with throwing functions without the hassle of fn(lambda, Requirements.empty()).

So for now I suggest to revert to the original version you proposed.

This comment has been minimized.

Copy link
@suztomo

suztomo Dec 10, 2019

Author Contributor

Created a ticket to followup https://issues.apache.org/jira/browse/BEAM-8942 .

The original code with try-catch is still in this PR.

@suztomo

This comment has been minimized.

Copy link
Contributor Author

suztomo commented Dec 2, 2019

@suztomo

This comment has been minimized.

Copy link
Contributor Author

suztomo commented Dec 10, 2019

@iemejia, @jbonofre

@jkff said this try-catch version is good (#10256 (comment)). Would you merge this if this looks good to you?

@suztomo

This comment has been minimized.

Copy link
Contributor Author

suztomo commented Dec 12, 2019

R: @lukecwik
(from OWNERS file)

@pabloem

This comment has been minimized.

Copy link
Member

pabloem commented Dec 16, 2019

Let me see if @chamikaramj can review

@pabloem pabloem requested a review from chamikaramj Dec 16, 2019
@chamikaramj

This comment has been minimized.

Copy link
Contributor

chamikaramj commented Dec 16, 2019

LGTM. Thanks.

@chamikaramj chamikaramj merged commit bb00774 into apache:master Dec 16, 2019
5 checks passed
5 checks passed
Java ("Run Java PreCommit") SUCCESS
Details
JavaPortabilityApi ("Run JavaPortabilityApi PreCommit") SUCCESS
Details
Java_Examples_Dataflow ("Run Java_Examples_Dataflow PreCommit") SUCCESS
Details
RAT ("Run RAT PreCommit") SUCCESS
Details
Spotless ("Run Spotless PreCommit") SUCCESS
Details
@suztomo suztomo deleted the suztomo:fileio_javadoc branch Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.