Skip to content
This repository has been archived by the owner on Oct 29, 2024. It is now read-only.

Add a client InfluxDBClusterClient to handle a cluster of InfluxDB servers #148

Merged
merged 5 commits into from
Apr 24, 2015

Conversation

cannium
Copy link
Contributor

@cannium cannium commented Apr 2, 2015

No description provided.

@cannium cannium mentioned this pull request Apr 2, 2015
7 tasks
@cannium cannium force-pushed the InfluxDBClusterClient branch from dfc1025 to 6ce631f Compare April 2, 2015 05:49
@cannium
Copy link
Contributor Author

cannium commented Apr 3, 2015

I'll try to figure out how to properly test the class. But before that I must fix my InfluxDB cluster where I seems to hit bugs.

@gst
Copy link
Contributor

gst commented Apr 9, 2015

What would be incredible is a(some) test case(s), against a real cluster, where you "kill" (in a way or another) some of the cluster nodes and you assert that queries are always executed (and get the correct answer) despite some of the nodes being killed/shutdown..

I have to agree/say that it's not necessarily easy to get that.. Doing it with mocks could be quite more easy to get working.. to be seen..

@cannium cannium force-pushed the InfluxDBClusterClient branch from 6ce631f to 74439c4 Compare April 10, 2015 07:32
@cannium
Copy link
Contributor Author

cannium commented Apr 10, 2015

Modified code pushed, tests and docs comming

@cannium cannium force-pushed the InfluxDBClusterClient branch 2 times, most recently from f814fd7 to 8e45a8e Compare April 10, 2015 09:37
@cannium cannium force-pushed the InfluxDBClusterClient branch from 8e45a8e to 91eb6d4 Compare April 10, 2015 10:34
@cannium
Copy link
Contributor Author

cannium commented Apr 13, 2015

Seems the failure isn't caused by this patch set, how can I trigger a retest?

@aviau
Copy link
Collaborator

aviau commented Apr 13, 2015

You can't. I just did!

@aviau
Copy link
Collaborator

aviau commented Apr 13, 2015

I intend to review this soon, sorry for the delay!

try:
cluster.query('Fail')
except InfluxDBServerError:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

You could/should also "assert the whole try/except block" with :

with self.assertRaises(InfluxDBServerError) as ctx:
    cluster.query('Fail')

?
EDIT: the as ctx isn't required/necessary as you don't assert anything specifically on the exception instance itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@aviau
Copy link
Collaborator

aviau commented Apr 16, 2015

Looks great, but my one issue with this is that I wonder how other clients will be able to use it.

For example the DataFrameClient and the SeriesHelper.

Would there be an acceptable way of including this in the InfluxDBClient?

@cannium
Copy link
Contributor Author

cannium commented Apr 17, 2015

By setting (client_base_class=xxx)[https://github.com//pull/148/files#diff-6f7b00b2898af5d67cc2a1ff0346c331R485] I suppose?

@aviau
Copy link
Collaborator

aviau commented Apr 17, 2015

Oh, I had overlooked! Makes sense!

However, other clients may need other things in the init.

Also, maybe we should use from_DSN?

@cannium
Copy link
Contributor Author

cannium commented Apr 20, 2015

Seems current from_DSN code in current code can not handle connection strings like influxdb://username:password@host1:port1,username:password@host2:port2,username:password@host3:port3/database_name, as in some other db.
What I'm thinking about is to create a function like create_new_client, if the connection string given includes a single server, returns an instance of InfluxDBClient, otherwise return an instance of InfluxDBClusterClient. What do you think?

@aviau
Copy link
Collaborator

aviau commented Apr 23, 2015

@cannium Sorry for the delay.

It looks like this is ready to be merged! Can I proceed? :)

@cannium
Copy link
Contributor Author

cannium commented Apr 23, 2015

Sure, go ahead

@aviau aviau merged commit 4555e6a into influxdata:master Apr 24, 2015
@aviau
Copy link
Collaborator

aviau commented Apr 24, 2015

Thank you for your contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants