Skip to content

Downgrade geoip2, exclude com.google.http-client.#2741

Merged
fjy merged 1 commit intoapache:masterfrom
gianm:examples-wiki
Mar 26, 2016
Merged

Downgrade geoip2, exclude com.google.http-client.#2741
fjy merged 1 commit intoapache:masterfrom
gianm:examples-wiki

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented Mar 25, 2016

Reverts "Update com.maxmind.geoip2 to 2.6.0" and exclude the google http client
from com.maxmind.geoip2. This should satisfy the original need from #2646 (wanting
to run Druid along with an upgraded com.google.http-client) while preventing
Jackson conflicts pointed out in #2717.

This reverts commit 21b7572.

Fixes #2717.

@gianm gianm added the Bug label Mar 25, 2016
@gianm gianm added this to the 0.9.1 milestone Mar 25, 2016
@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Mar 25, 2016

@erikdubbelboer does this approach work for you?

Reverts "Update com.maxmind.geoip2 to 2.6.0" and exclude the google http client
from com.maxmind.geoip2. This should satisfy the original need from apache#2646 (wanting
to run Druid along with an upgraded com.google.http-client) while preventing
Jackson conflicts pointed out in apache#2717.

Fixes apache#2717.

This reverts commit 21b7572.
@gianm gianm changed the title Revert "Update com.maxmind.geoip2 to 2.6.0" and exclude the google http client. Downgrade geoip2, exclude com.google.http-client. Mar 25, 2016
).get()) {
// 404 is ok here, that means it was already deleted
if (!HttpStatusCodes.isSuccess(returnCode.get()) || HttpStatusCodes.STATUS_CODE_NOT_FOUND != returnCode.get()) {
if (!httpStatusIsSuccess(returnCode.get()) || !httpStatusIsNotFound(returnCode.get())) {
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.

@drcrallen is this right? I think the || should be &&

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.

Or is correct.

Normal logic is success only.

Here are 404 is also a "success"

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.

Oh wai, let me look.

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.

Yeah, this is supposed to be any of 200 202 or 404 essentially. Looking into why unit tests didn't report weird stuff here

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.

Thanks for the catch. Will have unit test fix in soon

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.

test Fixed in #2743

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Mar 25, 2016

👍

1 similar comment
@xvrl
Copy link
Copy Markdown
Member

xvrl commented Mar 25, 2016

👍

@drcrallen
Copy link
Copy Markdown
Contributor

Tests run: 16, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 29.569 sec <<< FAILURE! - in io.druid.indexing.overlord.RemoteTaskRunnerTest
testRunExistingTaskThatHasntStartedRunning(io.druid.indexing.overlord.RemoteTaskRunnerTest)  Time elapsed: 1.186 sec  <<< FAILURE!
java.lang.AssertionError: expected:<SUCCESS> but was:<FAILED>
    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failNotEquals(Assert.java:743)
    at org.junit.Assert.assertEquals(Assert.java:118)
    at org.junit.Assert.assertEquals(Assert.java:144)
    at io.druid.indexing.overlord.RemoteTaskRunnerTest.testRunExistingTaskThatHasntStartedRunning(RemoteTaskRunnerTest.java:160)

@drcrallen drcrallen closed this Mar 26, 2016
@drcrallen drcrallen reopened this Mar 26, 2016
@fjy fjy merged commit 3c4691a into apache:master Mar 26, 2016
@erikdubbelboer
Copy link
Copy Markdown
Contributor

@gianm I'm on holiday at the moment so I haven't had time to test it yet.

@erikdubbelboer
Copy link
Copy Markdown
Contributor

@gianm I finally had time to test this, it works perfectly.

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Apr 6, 2016

@erikdubbelboer thanks, that's good to hear!

@gianm gianm deleted the examples-wiki branch April 6, 2016 18:49
erikdubbelboer added a commit to atomx/druid that referenced this pull request Sep 11, 2016
Also excludes the correct artifacts from apache#2741
erikdubbelboer added a commit to erikdubbelboer/druid that referenced this pull request Oct 2, 2016
Also excludes the correct artifacts from apache#2741
erikdubbelboer added a commit to erikdubbelboer/druid that referenced this pull request Oct 9, 2016
Also excludes the correct artifacts from apache#2741
erikdubbelboer added a commit to erikdubbelboer/druid that referenced this pull request Nov 16, 2016
Also excludes the correct artifacts from apache#2741
nishantmonu51 pushed a commit that referenced this pull request Nov 16, 2016
Also excludes the correct artifacts from #2741
fundead pushed a commit to fundead/druid that referenced this pull request Dec 7, 2016
Also excludes the correct artifacts from apache#2741
dgolitsyn pushed a commit to metamx/druid that referenced this pull request Feb 14, 2017
Also excludes the correct artifacts from apache#2741
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants