Bug fix, timestamp fix, documentation update #4

Merged
merged 11 commits into from Jan 13, 2012

2 participants

@roycamp

Fixed serverIndex Null Pointer bug
Preserve flume timestamp if it exists
Updated documentation

roycamp added some commits Dec 14, 2011
@roycamp roycamp Removing serverIndex as this.servers.get(this.serverIndex++) causes A…
…rray Index Out of Bounds when more than one server is supplied. This increment functionality isn't needed since server failures result in the server being removed from the server list and re-added after the retry period.
6730479
@roycamp roycamp Added example usage for multiple servers.
Added Troubleshooting section.
2f18a36
@roycamp roycamp Formatting fixes 8655add
@roycamp roycamp Preserve timestamp from when the event was generated if it exists. 607f809
@roycamp roycamp Fix key generation to use timestamp of event; still handles empty/mis…
…sing time.
7502fa8
@thobbs thobbs commented on the diff Jan 9, 2012
...ava/org/apache/cassandra/plugins/CassandraClient.java
@@ -165,7 +163,7 @@ String get() throws NoServersAvailableException {
if(this.servers.isEmpty()) {
throw new NoServersAvailableException();
}
- return this.servers.get(this.serverIndex++);
+ return this.servers.get(0);
@thobbs
Owner
thobbs added a line comment Jan 9, 2012

So, just incrementing serverIndex without making sure it hasn't gone off the end of the list was definitely broken, but we do want to cycle through the servers in our list. If we always use 0, that won't happen.

I can make the change, but we probably just need to modulus by serverList.size().

@roycamp
roycamp added a line comment Jan 9, 2012

My understanding is that ServerSet.get() is only called from CassandraClient.open(). CassandraClient.open() is only called during start up and when a Cassandra exception is thrown which results in ServerSet.markDead(). markDead() removes the server from the servers array thus causing the remaining servers to shift up in the array. When the markDead() timeout expires, the server is added onto the end of the servers array. This is how the cycling of servers is done. Is there a scenario where open() is called which I am missing?

@thobbs
Owner
thobbs added a line comment Jan 9, 2012

Ah, you're totally correct. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@thobbs thobbs commented on the diff Jan 9, 2012
...org/apache/cassandra/plugins/SimpleCassandraSink.java
@@ -63,7 +63,10 @@ public void open() throws IOException {
@Override
public void append(Event event) throws IOException, InterruptedException {
- long timestamp = System.currentTimeMillis() * MILLI_TO_MICRO;
+ // Preserve timestamp from when the event was generated
+ long timestamp = event.getTimestamp();
@thobbs
Owner
thobbs added a line comment Jan 9, 2012

The main problem with doing this is that the TimeUUID we generate just below this won't use the same timestamp, so columns in the index rows won't necessarily match up with the time range in the row key.

It's possible to create a TimeUUID given a timestamp, but we would need to swap out UUID libraries and use something like this: https://github.com/rantav/hector/blob/master/core/src/main/java/me/prettyprint/cassandra/utils/TimeUUIDUtils.java

@roycamp
roycamp added a line comment Jan 9, 2012

Ah, good catch. I only accounted for the row key not the column TimeUUID. I will commit a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@roycamp

I merged the most recent updates (maven support) -- hopefully did this right.
Added TimeUUID generation support for column names based on the event timestamp.

@roycamp

Do I need to include the UUID jar (from http://johannburkard.de/software/uuid/) in the lib folder or will the maven dependency handle it?

@thobbs
Owner

If the build were set up to make a single uber-jar, then we could forgo adding it to lib (and remove the requirement for the user to add multiple jars to the classpath). I'll merge this in and then see if I can make that happen.

Thanks!

@thobbs thobbs merged commit d4cd56d into thobbs:master Jan 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment