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

Bump chill version to 0.8.3 #1634

Merged
merged 2 commits into from
Jan 18, 2017
Merged

Bump chill version to 0.8.3 #1634

merged 2 commits into from
Jan 18, 2017

Conversation

ttim
Copy link
Collaborator

@ttim ttim commented Jan 17, 2017

In order to have https://github.com/twitter/summingbird working with new storm API we need to depend on kryo 3.0 and therefore one chill 0.8.x. This PR bumps chill dependency to 0.8.3.

@johnynek
Copy link
Collaborator

This may change the serialization of things that are using kryo for storage (which they shouldn't do, but people don't always do the right thing).

We should definitely do a version bump here.

We should definitely not publish a new version, in my opinion, until we can do 2.12 at the same time.

@ttim
Copy link
Collaborator Author

ttim commented Jan 17, 2017

I agree on both. Also I need to investigate travis build failure.

@johnynek
Copy link
Collaborator

Issue is:

should Default serialization should have tokens *** FAILED ***
[info]   java.lang.ClassNotFoundException: void

looks like a legit test failure. I guess something changed and void is somehow showing up and breaking that test.

@ttim ttim force-pushed the bump_chill_version branch from 3b37358 to 9b657b0 Compare January 18, 2017 01:27
@ttim
Copy link
Collaborator Author

ttim commented Jan 18, 2017

@johnynek Since 3.x (actually since some 2.x version) Kryo includes serializer for void by default in any Kryo instance. This test started to fail because void(Unit in case of scala) was not in a list of primitive types.

I added void to the list, but I'm not 100% sure how legitimate this change is.

If we are going to release it with 2.12 release should we merge this change to develop branch?

@johnynek
Copy link
Collaborator

seems fine to me.

I think this looks good now. I assume you want to merge so you can do branch publish to get summingbird tested with storm 1.0.2?

@ttim
Copy link
Collaborator Author

ttim commented Jan 18, 2017

@johnynek I've done internal twitter release already, so it's better for me to keep it merged but I'm ok either way.

What I really need is - to release scalding with new chill to bump scalding version in summingbird branch to make it mergeable (otherwise two versions of kryo come to summingbird classpath). Also I want to bump scalding dependency in storehaus before storehaus release.

@johnynek
Copy link
Collaborator

johnynek commented Jan 18, 2017 via email

@ttim ttim requested review from johnynek and isnotinvain January 18, 2017 15:08
@ttim ttim merged commit b0ed1be into twitter:develop Jan 18, 2017
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