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

Allow to connect to several TSD's #44

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
@adrien-mogenet
Contributor

adrien-mogenet commented Jun 21, 2012

Simple add to allow the user to provide several TSD's URI from the CLI.
tcollector will then trying to connect randomly to a server, and to another one if first is not available.

@adrien-mogenet

This comment has been minimized.

Show comment
Hide comment
@adrien-mogenet

adrien-mogenet Jun 21, 2012

Owner

Perhaps keeping this option for backward compatibility reasons ?

Perhaps keeping this option for backward compatibility reasons ?

This comment has been minimized.

Show comment
Hide comment
@tsuna

tsuna Jun 21, 2012

Yeah, please don't change existing command line arguments, they are the public interface of tcollector to the outside world.

I'd recommend adding a new flag that takes your comma-separated list.

  • if --hosts is specified:
    • if --host is still set to its default value: ignore it and use only what you got from --hosts
    • else: --host was set, then simply add the host:port from --host / --port at the beginning of the list you obtained from --hosts.
  • else: use only what you got from --host / --port.

This way it's 100% backwards compatible.

Yeah, please don't change existing command line arguments, they are the public interface of tcollector to the outside world.

I'd recommend adding a new flag that takes your comma-separated list.

  • if --hosts is specified:
    • if --host is still set to its default value: ignore it and use only what you got from --hosts
    • else: --host was set, then simply add the host:port from --host / --port at the beginning of the list you obtained from --hosts.
  • else: use only what you got from --host / --port.

This way it's 100% backwards compatible.

@adrien-mogenet

This comment has been minimized.

Show comment
Hide comment
@adrien-mogenet

adrien-mogenet Jun 22, 2012

Contributor

My last commit should ensure 100% backward compatibility. Available connections are also represented as tuples, as it's could be more convenient to handle python objects rather than raw strings.

Contributor

adrien-mogenet commented Jun 22, 2012

My last commit should ensure 100% backward compatibility. Available connections are also represented as tuples, as it's could be more convenient to handle python objects rather than raw strings.

@adrien-mogenet

This comment has been minimized.

Show comment
Hide comment
@adrien-mogenet

adrien-mogenet Jun 28, 2012

Owner

Oops, should be a different pull request ! Don't know how to remove a commit from a pull request....

For this commit, I think it's safer to use the user defined python.

Owner

adrien-mogenet commented on be31699 Jun 28, 2012

Oops, should be a different pull request ! Don't know how to remove a commit from a pull request....

For this commit, I think it's safer to use the user defined python.

@adrien-mogenet

This comment has been minimized.

Show comment
Hide comment
@adrien-mogenet

adrien-mogenet Jun 28, 2012

Contributor

I don't know if it's in your framework's philosophy, but I added the possibility to configure enabled collectors through a configuration file. This choice has been motivated since it could be more convenient to handle package/configuration deployment (I mean with tools such as Puppet or Chef) than moving/renaming list of collector files beginning with a dot.
By the way, even if these tools are able to trigger service restart, tcollector will automatically reload itself this configuration file when required.

Contributor

adrien-mogenet commented Jun 28, 2012

I don't know if it's in your framework's philosophy, but I added the possibility to configure enabled collectors through a configuration file. This choice has been motivated since it could be more convenient to handle package/configuration deployment (I mean with tools such as Puppet or Chef) than moving/renaming list of collector files beginning with a dot.
By the way, even if these tools are able to trigger service restart, tcollector will automatically reload itself this configuration file when required.

@tsuna

This comment has been minimized.

Show comment
Hide comment
@tsuna

tsuna Jan 19, 2013

Member

Hmm, this pull request sort of fell through the cracks. What do you think if I pull it as tsuna/tcollector@5f7046a – it contains a few fixes:

  • Shuffle the host list and pop the first item, instead of shuffling it each time. This way we preserve the order of the list given on the command-line for the first iteration. Otherwise the order on the command-line is meaningless, which may be surprising. Maybe we don't want to shuffle the list at all, and keep iterating on the same list as-is.
  • Properly split the host:port with respect to IPv6.
  • Consider that a host/port pair is non-default if the host or the port differ, not and.
Member

tsuna commented Jan 19, 2013

Hmm, this pull request sort of fell through the cracks. What do you think if I pull it as tsuna/tcollector@5f7046a – it contains a few fixes:

  • Shuffle the host list and pop the first item, instead of shuffling it each time. This way we preserve the order of the list given on the command-line for the first iteration. Otherwise the order on the command-line is meaningless, which may be surprising. Maybe we don't want to shuffle the list at all, and keep iterating on the same list as-is.
  • Properly split the host:port with respect to IPv6.
  • Consider that a host/port pair is non-default if the host or the port differ, not and.

@ghost ghost assigned tsuna Jan 19, 2013

@tsuna

This comment has been minimized.

Show comment
Hide comment
@tsuna

tsuna Jan 20, 2013

Member

There was a small bug I introduced in the commit I referred to above, I'm planning to merge this one instead: tsuna/tcollector@047f7f8

Member

tsuna commented Jan 20, 2013

There was a small bug I introduced in the commit I referred to above, I'm planning to merge this one instead: tsuna/tcollector@047f7f8

@adrien-mogenet

This comment has been minimized.

Show comment
Hide comment
@adrien-mogenet

adrien-mogenet Jan 20, 2013

Contributor

Hmm... picking a TSD randomly for first iteration was a first kind of load balancing solution without adding extra load balancer. What do you think?

Contributor

adrien-mogenet commented Jan 20, 2013

Hmm... picking a TSD randomly for first iteration was a first kind of load balancing solution without adding extra load balancer. What do you think?

@tsuna

This comment has been minimized.

Show comment
Hide comment
@tsuna

tsuna Jan 20, 2013

Member

True, I didn't see it this way, but I see how this makes sense. I'll change the code to shuffle the list early on.

Member

tsuna commented Jan 20, 2013

True, I didn't see it this way, but I see how this makes sense. I'll change the code to shuffle the list early on.

@tsuna

This comment has been minimized.

Show comment
Hide comment
@tsuna

tsuna Jan 20, 2013

Member

OK I added a couple lines in tsuna/tcollector@da080e4 (see L405-406 in the new file).

Member

tsuna commented Jan 20, 2013

OK I added a couple lines in tsuna/tcollector@da080e4 (see L405-406 in the new file).

@adrien-mogenet

This comment has been minimized.

Show comment
Hide comment
@adrien-mogenet

adrien-mogenet Jan 20, 2013

Contributor

Ok, let's merge :)

Contributor

adrien-mogenet commented Jan 20, 2013

Ok, let's merge :)

@adrien-mogenet

This comment has been minimized.

Show comment
Hide comment
@adrien-mogenet

adrien-mogenet Jan 21, 2013

Contributor

A few words anyway: take note that blacklisting hosts might not be a good choice at all; for instance when you're rolling restarting all your TSDs, load could not be evenly spread (no connection on the last restarted TSD).

Contributor

adrien-mogenet commented Jan 21, 2013

A few words anyway: take note that blacklisting hosts might not be a good choice at all; for instance when you're rolling restarting all your TSDs, load could not be evenly spread (no connection on the last restarted TSD).

@tsuna

This comment has been minimized.

Show comment
Hide comment
@tsuna

tsuna Feb 18, 2013

Member

Merged upstream as da080e4

Member

tsuna commented Feb 18, 2013

Merged upstream as da080e4

@tsuna tsuna closed this Feb 18, 2013

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