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 clj-time version to 0.8.0 #76

Closed
wants to merge 2 commits into from
Closed

Bump clj-time version to 0.8.0 #76

wants to merge 2 commits into from

Conversation

juskrey
Copy link

@juskrey juskrey commented Sep 19, 2014

Old clj-time version produces a nasty issue in case new version of the lib is used along with PigPen.
See clj-time/clj-time#124

Some tests were failing due to the fact Clojure maps are not guaranteed to preserve kv pairs order. Array- map are doing that. What structure behaviour to choose is up to the developer.

@@ -21,7 +21,7 @@
pigpen.extensions.core))

(deftest test-pp-str
(let [data {:a "very long string" :b "another really really long string" :c [1 2 3]}]
(let [data (array-map :a "very long string" :c [1 2 3] :b "another really really long string")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these changed? Did the change in clj-time pull in clojure 1.6.0?

Copy link
Author

Choose a reason for hiding this comment

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

Declared maps {} are not preserving key-value pairs order, array-maps do
this. So this fails one one machine, but may pass on other. Sorry, did not
clarify this in first comment, had to edit.

On Fri, Sep 19, 2014 at 7:28 PM, Matt Bossenbroek notifications@github.com
wrote:

In pigpen-core/src/test/clojure/pigpen/extensions/core_test.clj:

@@ -21,7 +21,7 @@
pigpen.extensions.core))

(deftest test-pp-str

  • (let [data {:a "very long string" :b "another really really long string" :c [1 2 3]}]
  • (let [data (array-map :a "very long string" :c [1 2 3] :b "another really really long string")]

Why are these changed?


Reply to this email directly or view it on GitHub
https://github.com/Netflix/PigPen/pull/76/files#r17794486.

@mbossenbroek
Copy link
Contributor

Looked some more & this does pull 1.6.0. I tried last week to upgrade to 1.6.0, but ran into some build/jar issue with instaparse that I don't remember now...

Were you able to run this on a hadoop cluster?

Longer term I plan to get rid of these dependencies altogether. I barely use them anyway.

@juskrey
Copy link
Author

juskrey commented Sep 20, 2014

Yes, this thing is working for me after fresh gradle build and fresh
uberjar, clojure 1.6.0. On AMR cluster.
I have instaparse linking issues with 1.7.0-alphas

On Fri, Sep 19, 2014 at 7:58 PM, Matt Bossenbroek notifications@github.com
wrote:

Looked some more & this does pull 1.6.0. I tried last week to upgrade to
1.6.0, but ran into some build/jar issue with instaparse that I don't
remember now...

Were you able to run this on a hadoop cluster?

Longer term I plan to get rid of these dependencies altogether. I barely
use them anyway.


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

@mbossenbroek
Copy link
Contributor

Pulled this in today & resolved the issue I was seeing before with 1.6.0. Turned out it was something loading weird in CCW, but fixed it nonetheless.

I went to try it out on our clusters & ran into some weird locking problem with the local mode. I'm going to try to merge the rx upgrade that was pending & give it another try after that.

I'm also removing the code that needed instaparse & clj-time, so these won't be a problem going forward either.

@mbossenbroek
Copy link
Contributor

Fixed by #78 - version 0.2.10

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