Skip to content

Move parquet cascading schemes to subprojects#1514

Open
rubanm wants to merge 8 commits into
developfrom
rubanm/move_cascading_schemes_to_subprojects
Open

Move parquet cascading schemes to subprojects#1514
rubanm wants to merge 8 commits into
developfrom
rubanm/move_cascading_schemes_to_subprojects

Conversation

@rubanm

@rubanm rubanm commented Feb 11, 2016

Copy link
Copy Markdown
Contributor

This moves all the parquet schemes to separate sub-projects than the parquet sources. This is mainly for easier upgrade to cascading3 and future versions perhaps. (As things stand now, we need to upgrade scalding-core in our cascading3 branch, before scalding-parquet for example.)

Comment thread build.sbt Outdated
@@ -394,17 +395,25 @@ lazy val scaldingParquet = module("parquet").settings(
exclude("com.twitter.elephantbird", "elephant-bird-pig")
exclude("com.twitter.elephantbird", "elephant-bird-core"),
"org.apache.thrift" % "libthrift" % "0.7.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we move this raw version string with the rest at the top of this file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated. The global thriftVersion is already present and set to 0.5.0, which I think should be okay.

@johnynek

Copy link
Copy Markdown
Contributor

👍

@ianoc

ianoc commented Feb 13, 2016

Copy link
Copy Markdown
Contributor

This seems useful for the cascading 3 upgrade, and makes sense there. Do we want it in the 0.16 release cycle ? Or keep it closer to the big painful cascading one?

@rubanm would this be hard to take as an update at twitter? (just as an exemplar of issues elsewhere possibly).

@ianoc

ianoc commented Feb 13, 2016

Copy link
Copy Markdown
Contributor

With the aliases and deps in place it should be handled mostly by ivy resolves I think?

Can we have the new types/classes be package private to scalding? They seem like an internal implementation detail as far as scalding is concerned, would be nice to not have them in our ABI if we could? thoughts @johnynek ?

@rubanm

rubanm commented Feb 13, 2016

Copy link
Copy Markdown
Contributor Author

Re cascading3, I could cherry pick the sbt update commit (moves from Build.scala to build.sbt) to cascading3 branch, and apply this on top of it. If we don't merge this to develop, it will be one more thing to look at in the diff in the eventual cascading3 -> develop merge.

Yes given the pre-existing sub-projects will still pull in classes in the corresponding new *-cascading ones, the upgrade should be fine I think. I'm in favor of merging to develop (and cherry picking to cascading3 branch) if this seems reasonable as a general thing to do.

@rubanm

rubanm commented Feb 13, 2016

Copy link
Copy Markdown
Contributor Author

Some of Schemes are consumed at Twitter in multiformat sources, for example. The rest could be made package private I think.

@johnynek

Copy link
Copy Markdown
Contributor

@rubanm we will need help from you and @isnotinvain to make sure we keep twitter up to date and not let any forks happen!

So, if you think you can get the merge in at Twitter, I'm fine with a merge for 0.16.

@ianoc

ianoc commented Feb 13, 2016

Copy link
Copy Markdown
Contributor

Yep, good way to put it. Merge when green from me if the hassle level is ok
for you guys to handle to merge.

On Friday, February 12, 2016, P. Oscar Boykin notifications@github.com
wrote:

@rubanm https://github.com/rubanm we will need help from you and
@isnotinvain https://github.com/isnotinvain to make sure we keep
twitter up to date and not let any forks happen!

So, if you think you can get the merge in at Twitter, I'm fine with a
merge for 0.16.


Reply to this email directly or view it on GitHub
#1514 (comment).

Sent from Gmail Mobile

@rubanm

rubanm commented Feb 16, 2016

Copy link
Copy Markdown
Contributor Author

@ianoc @johnynek Makes sense. I'll wait for a green sandbox internally before merging. (So we block this on the new algebird version to release internally at Twitter first, with the related breaking tests fixed.) I'll just cherry pick this to the cascading3 branch meanwhile to keep that going.

@rubanm rubanm closed this Feb 16, 2016
@rubanm rubanm reopened this Feb 16, 2016
@rubanm

rubanm commented Feb 16, 2016

Copy link
Copy Markdown
Contributor Author

Sorry, closed by mistake.

@johnynek

Copy link
Copy Markdown
Contributor

should we merge this now? I kind of lost track of it.

@CLAassistant

CLAassistant commented Nov 16, 2019

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

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.

4 participants