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

[accumulo] update binding to 1.7.2 #787

Merged
merged 1 commit into from Oct 8, 2017
Merged

Conversation

madrob
Copy link
Contributor

@madrob madrob commented Jun 30, 2016

No description provided.

@busbey
Copy link
Collaborator

busbey commented Jun 30, 2016

are folks likely to want to compare 1.6 to 1.7?

@@ -24,6 +24,6 @@ log4j.appender.stderr.layout=org.apache.log4j.PatternLayout
log4j.appender.stderr.layout.conversionPattern=%d{yyyy/MM/dd HH:mm:ss} %-5p %c %x - %m%n

# Suppress messages from ZooKeeper
log4j.logger.com.yahoo.ycsb.db.accumulo=INFO
log4j.logger.com.yahoo.ycsb.db.accumulo=DEBUG
Copy link
Collaborator

Choose a reason for hiding this comment

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

was this on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I left the root logger at INFO so there is no user-facing change, but this makes future debugging easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah. ✅

@busbey
Copy link
Collaborator

busbey commented Jun 30, 2016

related to the "compare 1.6 to 1.7", can this updated client talk to a 1.6 cluster?

@busbey
Copy link
Collaborator

busbey commented Jun 30, 2016

the only concern I can see would be that we still haven't broken out versioned accumulo clients (as we do for e.g. hbase098 and hbase10). So when this change goes in, folks might not be able to use a single YCSB deployment to compare and Accumulo 1.6 deployment to Accumulo 1.7. If this client can talk to both 1.6 and 1.7 clusters, I'm less concerned (though I guess that means folks would still have a hard time comparing changes in the client-side code).

@busbey
Copy link
Collaborator

busbey commented Jun 30, 2016

one of the failures was the known flakey for Cassandra managing to start in time. the other one looks like maybe the Accumulo 1.7 minicluster might have similar issues? Is there a timeout we can make more forgiving?

@busbey
Copy link
Collaborator

busbey commented Jul 22, 2016

@madrob I've tried restarting the travis-ci run a few times and it's consistently failing to start in time in openjdk7.

Any opinions on the ease of comparing 1.6 and 1.7 @joshelser or @ctubbsii

@joshelser
Copy link
Contributor

Any opinions on the ease of comparing 1.6 and 1.7

I would guess that a 1.6 client can still talk to a 1.7 cluster (via public API), but not something I've tested directly either. I should really try to get more involved here too. Let me see if I can test it out locally.

@busbey
Copy link
Collaborator

busbey commented Jul 22, 2016

I believe this would change the client to be 1.7. Any idea if a 1.7 client can talk to a 1.6 cluster?

Even if they can, when comparing things would you want to see differences caused by client version changes between 1.6 and 1.7?

@joshelser
Copy link
Contributor

I believe this would change the client to be 1.7. Any idea if a 1.7 client can talk to a 1.6 cluster?

A similar (or slightly less) degree of confidence than 1.6->1.7 :)

Even if they can, when comparing things would you want to see differences caused by client version changes between 1.6 and 1.7?

Comparing things==perf? I think it would be nice to know if it's implying that a version X client can talk to a version X+1 cluster.

@risdenk
Copy link
Collaborator

risdenk commented Sep 27, 2016

Looks like this actually failed. Is this possible due to the version upgrade?

Running com.yahoo.ycsb.db.accumulo.AccumuloTest
2016/07/05 03:03:38 DEBUG com.yahoo.ycsb.db.accumulo.AccumuloTest  - starting minicluster
*** buffer overflow detected ***: /usr/lib/jvm/java-7-openjdk-amd64/jre/bin/java terminated
...

@busbey
Copy link
Collaborator

busbey commented Sep 27, 2016

I think maybe the 1.7 minicluster takes a little longer to get going, which gets us this failure. AFAICT it passes locally.

Given that Accumulo 1.6 is reaching the end of its life upstream, I think it's worth breaking this into two verisoned modules (or three, since 1.8 is out now).

@joshelser
Copy link
Contributor

I think it's worth breaking this into two verisoned modules (or three, since 1.8 is out now).

For running these tests, having the correct versions of the Accumulo client on the classpath? From the Accumulo API, we shouldn't need to do this.

@ctubbsii
Copy link
Contributor

The buffer overflow is probably the OpenJDK bug with the long hostnames Travis provides. See travis-ci/travis-ci#5227.

The workaround is to set a custom hostname and add that hostname to /etc/hosts ... or avoid the buggy openjdk in Travis entirely.

@busbey
Copy link
Collaborator

busbey commented Sep 27, 2016

I think it's worth breaking this into two verisoned modules (or three, since 1.8 is out now).

For running these tests, having the correct versions of the Accumulo client on the classpath? From the Accumulo API, we shouldn't need to do this.

yeah, for allowing folks to test the impact of changes in the Accumulo client. nothing to do with API use at first cut. We might later want to incorporate some of the newer client side APIs (e.g. KerberosToken).

@busbey
Copy link
Collaborator

busbey commented Sep 27, 2016

The workaround is to set a custom hostname and add that hostname to /etc/hosts ... or avoid the buggy openjdk in Travis entirely.

That work around would require us to get off of the container based infra (since it requires sudo). Did openjdk never provide a fixed version for openjdk7? I guess we could start a thread about switching to openjdk8 instead of openjdk7, but I'd want that broken out from an Accumulo PR since it would mean relying on just the oracle jdk7 for jdk7 checks.

@risdenk
Copy link
Collaborator

risdenk commented Sep 27, 2016

@busbey the "new" workaround may not require sudo.

travis-ci/travis-ci#5227 (comment)

addons:
  hosts:
    - myshorthost
  hostname: myshorthost

@busbey
Copy link
Collaborator

busbey commented Sep 27, 2016

the docs for hostname that @ctubbsii pointed to state that it requires sudo images.

@risdenk
Copy link
Collaborator

risdenk commented Sep 27, 2016

ah ok :(

@ctubbsii
Copy link
Contributor

Yeah, it requires sudo, unfortunately. Not sure when/if it matters that it can't use the container infrastructure.

It looks like it was fixed in Java 8. Since 7 is basically EOL, and it really only affects Docker users in most cases, I suspect it won't be patched in 7. And, even if it were, there's no guarantee when/if Travis would update.

@risdenk
Copy link
Collaborator

risdenk commented Feb 3, 2017

@joshelser or @madrob - Any plans to update this?

Copy link
Collaborator

@risdenk risdenk left a comment

Choose a reason for hiding this comment

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

Needs to at least be rebased

@joshelser
Copy link
Contributor

That work around would require us to get off of the container based infra (since it requires sudo). Did openjdk never provide a fixed version for openjdk7

Do you recall if this happened, @risdenk? Being lazy :)

@risdenk
Copy link
Collaborator

risdenk commented Feb 7, 2017

@joshelser not sure. I never checked.

@busbey
Copy link
Collaborator

busbey commented Sep 22, 2017

following up: we did not get off of the container based infra (it's the travis preferred option for projects). Also we now rely entirely on openjdk7 since oraclejdk7 went away. I don't know if there's a fix for openjdk7

busbey added a commit to busbey/YCSB that referenced this pull request Sep 22, 2017
@busbey busbey merged commit 82011d5 into brianfrankcooper:master Oct 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants