[BEAM-111] Move WritableCoder to hadoop-common by iemejia · Pull Request #2118 · apache/beam

iemejia · 2017-02-27T20:34:22Z

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify. (Even better, enable
Travis-CI on your fork and ensure the whole test matrix passes).
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

iemejia · 2017-02-27T20:35:30Z

R: @amitsela
This is an oldie, but now with the module for hadoop-common we can move this one there.
Please check the jackson dependency, I am not sure if I should make it transitive as I do.
Thanks.

coveralls · 2017-02-27T21:33:52Z

Changes Unknown when pulling 14dfc21 on iemejia:BEAM-111-move-writablecoder into ** on apache:master**.

asfbot · 2017-02-27T21:34:41Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7914/
--none--

amitsela

Added some comments, but mostly good.

amitsela · 2017-02-28T12:21:16Z

        </exclusion>
      </exclusions>
    </dependency>
+    <dependency>


Scope test ?

amitsela · 2017-02-28T12:23:33Z

  <description>Library to add shared Hadoop classes among Beam IOs.</description>

  <dependencies>
+    <dependency>


Is this being introduced for the first time because of WritableCoder moving into hadoop-common ? I guess because of Coder, right ?

Yes, that's the reason, do you think this is ok (considering that the other libraries depending on this one probably will have the same dependency) ?

It shouldn't matter since they all bring the same dependency, and I believe that WritableCoder should be in hadoop-common and not hdfs-io,

amitsela · 2017-02-28T12:25:02Z

+      <artifactId>beam-sdks-java-core</artifactId>
+    </dependency>
+
+    <dependency>


Not sure why this is provided, how is this different from the one you removed from io/hdfs ?

It was not provided, but I am afraid of the version of this leaking into the users of the common library. What do you think should I let it like it was (without the provided) ?

It was like that in hdfs-io, and if users need to use a different jackson version they should do so explicitly - the Spark runner does that.

iemejia · 2017-02-28T14:27:06Z

Ok fixed after comments and rebased.

coveralls · 2017-02-28T15:08:16Z

Coverage increased (+0.003%) to 69.187% when pulling 799cc6f on iemejia:BEAM-111-move-writablecoder into 2a23374 on apache:master.

asfbot · 2017-02-28T15:09:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7958/
--none--

iemejia · 2017-03-01T10:06:33Z

Rebased because there was a merge issue after changes in master.

coveralls · 2017-03-01T10:43:16Z

Coverage increased (+0.007%) to 69.196% when pulling da8b0ad on iemejia:BEAM-111-move-writablecoder into d66029c on apache:master.

asfbot · 2017-03-01T10:46:02Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8009/
--none--

coveralls · 2017-03-01T13:39:03Z

Coverage increased (+0.01%) to 69.203% when pulling 5eb2422 on iemejia:BEAM-111-move-writablecoder into d66029c on apache:master.

asfbot · 2017-03-01T13:39:30Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8011/
--none--

amitsela · 2017-03-01T19:28:59Z

@iemejia before I merge this, are you and @diptikul synchronized about #1994 ?
It looks as if Coders are moved around there as well.
CC: @dhalperi @ssisk @jbonofre

iemejia · 2017-03-01T20:40:23Z

We are not synchronized because these are different subjects, and this PR is not a blocker for that PR, what I am doing for the moment is just looking for Hadoop classes that are needed in more than one project and moving those to hadoop-common.
What I saw in the review is that the reviewers proposed to do this changes (use or enhance hadoop-common) afterwards, to add less noise to the current InputFormat PR.

amitsela · 2017-03-01T20:45:54Z

Ok, I guess it'll get merged into #1994 eventually. Thanks @iemejia

Merging.

iemejia · 2017-03-01T21:09:06Z

Thanks.

amitsela reviewed Feb 28, 2017

View reviewed changes

iemejia force-pushed the BEAM-111-move-writablecoder branch from 14dfc21 to 799cc6f Compare February 28, 2017 14:26

iemejia force-pushed the BEAM-111-move-writablecoder branch from 799cc6f to da8b0ad Compare March 1, 2017 10:05

[BEAM-111] Move WritableCoder to hadoop-common

5eb2422

iemejia force-pushed the BEAM-111-move-writablecoder branch from da8b0ad to 5eb2422 Compare March 1, 2017 13:05

asfgit closed this in a81c457 Mar 1, 2017

iemejia deleted the BEAM-111-move-writablecoder branch March 1, 2017 21:06

Conversation

iemejia commented Feb 27, 2017

Uh oh!

iemejia commented Feb 27, 2017

Uh oh!

coveralls commented Feb 27, 2017

Uh oh!

asfbot commented Feb 27, 2017

Uh oh!

amitsela left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iemejia commented Feb 28, 2017

Uh oh!

coveralls commented Feb 28, 2017

Uh oh!

asfbot commented Feb 28, 2017

Uh oh!

iemejia commented Mar 1, 2017

Uh oh!

coveralls commented Mar 1, 2017

Uh oh!

asfbot commented Mar 1, 2017

Uh oh!

coveralls commented Mar 1, 2017

Uh oh!

asfbot commented Mar 1, 2017

Uh oh!

amitsela commented Mar 1, 2017

Uh oh!

iemejia commented Mar 1, 2017

Uh oh!

amitsela commented Mar 1, 2017

Uh oh!

iemejia commented Mar 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amitsela left a comment •

edited

Loading