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

Conversation

geodimm
Copy link
Contributor

@geodimm geodimm commented Sep 13, 2015

This should enable the cluster client to properly switch databases and user credentials for all clients in the cluster, not just a random one. It doesn't look pretty and actually I'd like to suggest an alternative implementation of the InfluxDBClusterClient which will actually inherit the InfluxDBClient and instead of maintaining a list of InfluxDBClients, we can just keep the list of (host, port) tuples and alternate those on each request. This way we win a few things:

  • Less memory usage, obviously. There's no need to create 3 InfluxDBClient objects when we can get away with a single one;
  • We get access to all attributes of the InfluxDBClient (not just the callable ones);
  • Docstrings available for all methods. It also means that auto-completion will be available for developers;
  • Easier maintenance and avoiding bugs similar to this one.

Since some users of the library might be using the clients attribute of the cluster client current implementation, we can make the new one backwards compatible by having a list with only one element (self) as the clients attribute, i.e. self.clients = [self]

@aviau , @gst, @areski , any thoughts on that?

The methods switch_database and switch_user are supposed to be called on
all clients in the cluster, not just a random one.
@gst
Copy link
Contributor

gst commented Sep 13, 2015

seems totally legit to me, so +1 :)
Have you a branch already made with what you propose ?

@geodimm
Copy link
Contributor Author

geodimm commented Sep 14, 2015

Hi,
I haven't written anything yet, but I can come up with an initial draft tonight :)

@aviau
Copy link
Collaborator

aviau commented Sep 14, 2015

Thank you @georgijd

@aviau aviau force-pushed the master branch 2 times, most recently from 4b91f24 to 65292f2 Compare September 16, 2015 14:46
@geodimm
Copy link
Contributor Author

geodimm commented Sep 18, 2015

Hey guys, sorry for the delay, it's been a busy week :)
You can check out what I've come up with here: geodimm@e25576f

@aviau
Copy link
Collaborator

aviau commented Oct 7, 2015

@georgijd Can you transfer this onto this PR?

@aviau
Copy link
Collaborator

aviau commented Nov 13, 2015

As far as I understand, this is fixed in #253

@aviau aviau closed this Nov 13, 2015
@geodimm geodimm deleted the fix-issue-237 branch November 13, 2015 22:36
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