search

pchalamet / cassandra-sharp

high performance .NET driver for Apache Cassandra
114 stars 41 forks source link

Property File Snitch #58

Closed axle-h closed 11 years ago

axle-h commented 11 years ago

Added a new snitch that uses the xml configuration framework.

It's something that we require on our cassandra deployment for a future enhanchement that will see accurate datacentre failover. We're actually using GossipingPropertyFileSnitch but can live with updating App.Config on the clients as the network topology changes.

Only issue I can see is if discovery adds new nodes to the cluster, the xml properties will be out of date. Currently this throws an exception but since the datacentre and rack are available in system.peers, which is queried on node Discovery, could we add a way to dynamically update a snitch? Maybe add update method to iEndpointSnitch that is initialized in ClusterManager like:

 discoveryService.OnTopologyUpdate += snitch.Update;

I'm happy to make another push if you agree?

pchalamet commented 11 years ago

Using the same cassandra-topology.properties file as the one in C* config would be much better (alongside main app).

New nodes or unknown nodes would be handled as well with the default config - then no need to link discovery and snitch.

cassandra-topology.properties file could also be watched for changes to refresh snitch config dynamically.

axle-h commented 11 years ago

The problem with that approach is that you have to manually update the property file on every node. The way we have it setup is to use a gossiping property file so there's no single file that contains all node info. You don't then have to update the file on every node when you add a new one (we have 20 and are moving to 40). Also we're not running C* sharp from a C* node so would require remote file access which for security reasons is not a possibility. We would never rely on the default node option as it makes no sense to incorrectly bootstrap a new node to then undo it and repair once the topology is corrected. This is just how we use Cassandra of course. Your thoughts? On May 29, 2013 10:10 PM, "Pierre Chalamet" notifications@github.com wrote:

Using the same cassandra-topology.properties file as the one in C* config would be much better (alongside main app).

New nodes or unknown nodes would be handled as well with the default config - then no need to link discovery and snitch.

cassandra-topology.properties file could also be watched for changes to refresh snitch config dynamically.

— Reply to this email directly or view it on GitHubhttps://github.com/pchalamet/cassandra-sharp/pull/58#issuecomment-18646430 .

pchalamet commented 11 years ago

I think I was misunderstandood :) retrying…

I’m just saying that it would be far better to have the same file structure to describe the snitch as in C. Not saying they should be exactly the same nor C# clients should share servers with C*.

Actually, the whole point of your dev is to link an endpoint IP to snitch info:

    <Endpoints strategy="TokenRing" snitch="PropertyFile">

        <Server dc="DC1" rack="RAC3">127.0.0.1</Server>

        <Server dc="DC2" rack="RAC2">127.0.0.10</Server>

        <Server dc="DC3" rack="RAC1">127.0.0.100</Server>

Well, I’m just saying this info should be outside app.config and read from a file with the same structure as C* topology file.

This file would only be used client side and will clearly be distinct from the one on the C* servers – but with the same file structure to make things easy to manage.

axle-h commented 11 years ago

Ah I see. OK so the client doesn't need to be restarted on topology changes. I'm not sure I like it as will add a second level of config and using a config that may change but if that's the the way you'd rather go? On May 29, 2013 11:14 PM, "Pierre Chalamet" notifications@github.com wrote:

I think I was misunderstandood :) retrying…

I’m just saying that it would be far better to have the same file structure to describe the snitch as in C. Not saying they should be exactly the same nor C# clients should share servers with C*.

Actually, the whole point of your dev is to link an endpoint IP to snitch info:

127.0.0.1 127.0.0.10 127.0.0.100 Well, I’m just saying this info should be outside app.config and read from a file with the same structure as C\* topology file. This file would only be used client side and will clearly be distinct from the one on the C\* servers – but with the same file structure to make things easy to manage. — Reply to this email directly or view it on GitHubhttps://github.com/pchalamet/cassandra-sharp/pull/58#issuecomment-18649891 .
pchalamet commented 11 years ago

Yes. I really prefer to unify such file if possible. Having another file structure does not really make sense.

-----Original Message----- From: "axle-h" notifications@github.com Sent: ‎30/‎05/‎2013 01:03 To: "pchalamet/cassandra-sharp" cassandra-sharp@noreply.github.com Cc: "Pierre Chalamet" pierre@chalamet.net Subject: Re: [cassandra-sharp] Property File Snitch (#58)

Ah I see. OK so the client doesn't need to be restarted on topology changes. I'm not sure I like it as will add a second level of config and using a config that may change but if that's the the way you'd rather go? On May 29, 2013 11:14 PM, "Pierre Chalamet" notifications@github.com wrote:

I think I was misunderstandood :) retrying…

I’m just saying that it would be far better to have the same file structure to describe the snitch as in C. Not saying they should be exactly the same nor C# clients should share servers with C*.

Actually, the whole point of your dev is to link an endpoint IP to snitch info:

127.0.0.1 127.0.0.10 127.0.0.100 Well, I’m just saying this info should be outside app.config and read from a file with the same structure as C\* topology file. This file would only be used client side and will clearly be distinct from the one on the C\* servers – but with the same file structure to make things easy to manage. — Reply to this email directly or view it on GitHubhttps://github.com/pchalamet/cassandra-sharp/pull/58#issuecomment-18649891 . — Reply to this email directly or view it on GitHub.
axle-h commented 11 years ago

I've had another think about this today and a look at how the datastax Java driver deals with the snitch. I think they have it right. They don't try to guess anything and instead leave it up to C* to tell them the network topology on discovery for all nodes. Then there's no worries about getting it wrong. Only thing is that discovery would either be mandatory or set to only update the topology for nodes set in the config. How about that instead of properties altogether? Don't mind forking it if you don't agree? Alex On May 30, 2013 12:59 PM, "Pierre Chalamet" notifications@github.com wrote:

Yes. I really prefer to unify such file if possible. Having another file structure does not really make sense.

  • Pierre

-----Original Message----- From: "axle-h" notifications@github.com Sent: 30/05/2013 01:03 To: "pchalamet/cassandra-sharp" cassandra-sharp@noreply.github.com Cc: "Pierre Chalamet" pierre@chalamet.net Subject: Re: [cassandra-sharp] Property File Snitch (#58)

Ah I see. OK so the client doesn't need to be restarted on topology changes. I'm not sure I like it as will add a second level of config and using a config that may change but if that's the the way you'd rather go? On May 29, 2013 11:14 PM, "Pierre Chalamet" notifications@github.com wrote:

I think I was misunderstandood :) retrying…

I’m just saying that it would be far better to have the same file structure to describe the snitch as in C. Not saying they should be exactly the same nor C# clients should share servers with C*.

Actually, the whole point of your dev is to link an endpoint IP to snitch info:

127.0.0.1 127.0.0.10 127.0.0.100 Well, I’m just saying this info should be outside app.config and read from a file with the same structure as C\* topology file. This file would only be used client side and will clearly be distinct from the one on the C\* servers – but with the same file structure to make things easy to manage. — Reply to this email directly or view it on GitHub< https://github.com/pchalamet/cassandra-sharp/pull/58#issuecomment-18649891> . — Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/pchalamet/cassandra-sharp/pull/58#issuecomment-18675814 .

pchalamet commented 11 years ago

That’s the whole point of SystemPeersDiscoveryService…

Actually, this just grabs the bare minimal information required for connecting to new nodes but it really should query more info.

System.peers contains following columns at this time:

data_center, host_id, peer, rack, release_version, rpc_address, schema_, tokens

just change the select query in SystemPeersDiscoveryService – also strategies should handle something more elaborated than IPAddress.

That’s definitively better than a property file I agree. Topology file can be required client side in some circumstances; but really, I do not see a compelling reason to do that :-)

axle-h commented 11 years ago

Thanks for being patient with me. Yeah I know about the system.peers table and your discovery service based on it hence why I originally suggested you link it with the snitch so it can take advantage of the topology info in system.peers. would you accept a pull request from me if I do something like that and drop this property file stuff? Alex On May 30, 2013 9:29 PM, "Pierre Chalamet" notifications@github.com wrote:

That’s the whole point of SystemPeersDiscoveryService…

Actually, this just grabs the bare minimal information required for connecting to new nodes but it really should query more info.

System.peers contains following columns at this time:

data_center, host_id, peer, rack, release_version, rpcaddress, schema, tokens

just change the select query in SystemPeersDiscoveryService – also strategies should handle something more elaborated than IPAddress.

That’s definitively better than a property file I agree. Topology file can be required client side in some circumstances; but really, I do not see a compelling reason to do that :-)

— Reply to this email directly or view it on GitHubhttps://github.com/pchalamet/cassandra-sharp/pull/58#issuecomment-18706156 .

pchalamet commented 11 years ago

Sure!

axle-h commented 11 years ago

Here we go. Let me know what you think. I've been a bit cheeky and set the default snitch to the new DiscoverySnitch. I think that's the way forward and maybe even on the way to removing snitch extensibility altogether - i.e. why bother trying to guess the topology of nodes when C* can tell you exactly.

I've had to do something I don't like with the failover endpoint strategy, which is to set it up with a temporary structure first to deal with the discovery service queries. I don't like this but I think it's necessary as discovery can be turned off. If Discovery wasn't optional we could deal with this initial state better I think.

Alex

pchalamet commented 11 years ago

No snitch won't disappear as well strategy. Sometimes you just do not want to stick with what C* tells you. Say you know some important information about your network (interconnection between DC and IP distribution), you might want to have a custom snitch and/or strategy.

Regarding your pull request: could you stick with 1 dev at a time? This ticket is about adding property file snitch. And this is no more the case. Moreover, a new strategy appears and this has nothing to do with this subject. It's hard to review.

However - just to helps you going on:

Also, non core functionnalities (that are not production ready) should go to cassandra-sharp-contrib. So please, consider you have access to extensibility api if you'd like to add a new strategy/snitch for the moment.

Thanks.

pchalamet commented 11 years ago

I've merge part of your pull request (discovery datacenter & rack). Also added a way to help you inject primaryDataCenter attribute in the strategy config (see EndpointsConfig.Extensions).

Now, go ahead, cleanup the your strategy and this will be ok.

axle-h commented 11 years ago

Apologies. I am new to Github, the pull request was automatically changed as I reverted the original commit and added the other. I felt it carried on the conversation here hence why I didn't then close it and re-open.

I agree with all of your points other than the idea of a client side Snitch. I knew you wouldn't be happy with my setting for default snitch, tbh I forgot that I'd left that in before I pressed commit :-). Only advantage I see to having snitch on client is the possibility of slightly more performance as you're essentially trying to implement the same behavior of the C* snitch. There is no point having a different view of the topology to the C* snitch. The results of which are available in the local & peers tables.

In terms of cleaning up my strategy I can:

Is that all? Do you want me to stick it in a new request? In this repo or move to contrib? The only thing outstanding and concerning this repo exclusively is the Update method in IEndpointSnitch. I can't add a Snitch based off discovery without it. Are you still not happy with it, or do you have a better idea?

pchalamet commented 11 years ago

Hi,

Let's stick with this pull request for the moment.

I do not agree snitch is done client side. Actually, distance computation is done using IP. This is not always what you want client side. A distance is something vague and can be latency instead for example. And you might want to reuse it in other strategy.

Endpoints strategies are already updated with discovered nodes. There is no point in updating snitch. A snitch is only there to compute a distance (and that can be a latency or something else) and the strategy use that to choose a node. I will look at your strategy implementation because there is something I can't grasp right here.

Anyway, yes for all points regarding the clean up of the strategy.

Le Jun 3, 2013 à 11:24 AM, axle-h notifications@github.com a écrit :

Apologies. I am new to Github, the pull request was automatically changed as I reverted the original commit and added the other. I felt it carried on the conversation here hence why I didn't then close it and re-open.

I agree with all of your points other than the idea of a client side Snitch. I knew you wouldn't be happy with my setting for default snitch, tbh I forgot that I'd left that in before I pressed commit :-). Only advantage I see to having snitch on client is the possibility of slightly more performance as you're essentially trying to implement the same behavior of the C* snitch. There is no point having a different view of the topology to the C* snitch. The results of which are available in the local & peers tables.

In terms of cleaning up my strategy I can:

Is that all? Do you want me to stick it in a new request? In this repo or move to contrib? The only thing outstanding and concerning this repo exclusively is the Update method in IEndpointSnitch. I can't add a Snitch based off discovery without it. Are you still not happy with it, or do you have a better idea?

— Reply to this email directly or view it on GitHubhttps://github.com/pchalamet/cassandra-sharp/pull/58#issuecomment-18830205 .

pchalamet commented 11 years ago

ok got it :) Seems I mess up when saying "strategies should handle something more elaborated than IPAddress" above. This should have been read as "snitch should handle something more elaborated than IPAddress".

Change ISnitch to use Peer instead of IPAddress. This should solve the Update problem.

pchalamet commented 11 years ago

oups, harder than anticipated. Let me think about this.

pchalamet commented 11 years ago

after all, you can just avoid using snitch after all - it's useless in your case.

axle-h commented 11 years ago

Yeah can do... seems silly when there's a snitch object to program a snitch like behaviour into an end point strategy though. Shall I just use extensibility API for this? The config extensions is enough for me to implement it on my own now anyway. Alex On Jun 4, 2013 8:09 PM, "Pierre Chalamet" notifications@github.com wrote:

after all, you can just avoid using snitch after all - it's useless in your case.

— Reply to this email directly or view it on GitHubhttps://github.com/pchalamet/cassandra-sharp/pull/58#issuecomment-18931951 .

pchalamet commented 11 years ago

Yeah this sucks. But frankly, I’m not really willing to break this at the moment...

Finally, I mostly agree with you about snitch utility – it’s not that usefull client side since in can only cope with IPAddress.

It is pretty much useless when non part of the ring.

Probably only strategies should exist. Frankly, I’m only using Nearest/RackInferring, this won’t hurt me if snitch disappears for whatever reason.

Don’t know what you think about this, but snitch interface is limited actually and probably does not worth it. Good candidate for [Obsolete].

axle-h commented 11 years ago

Yeah will be a lot of work and a lot of broken interfaces. The datastax drivers are no where near production ready but they follow a good standard for endpoint selection. Instead of a strategy and a snitch, they just use a LoadBalancingPolicy e.g. DCAwareRoundRobinPolicy, which combines both. They also listen for notifications through the binary protocol. I'm not saying this is the best way to do it, but is definitely a step in the right direction. Thanks for your help getting config extensions and topology discovery in there.

Alex

pchalamet commented 11 years ago

Yea, this is probably the way to go regarding policies. Thanks for your feedback on this, you managed to changed my mind in this :)

For different reason, I've been reluctant to listen for notification - I do not see the point when info can be queried and not really sure about overall reliability. I'm probably wrong and this probably reflect my very own usage of C*.

-----Original Message----- From: "Alex Haslehurst" notifications@github.com Sent: ‎05/‎06/‎2013 10:47 To: "pchalamet/cassandra-sharp" cassandra-sharp@noreply.github.com Cc: "Pierre Chalamet" pierre@chalamet.net Subject: Re: [cassandra-sharp] Property File Snitch (#58)

Yeah will be a lot of work and a lot of broken interfaces. The datastax drivers are no where near production ready but they follow a good standard for endpoint selection. Instead of a strategy and a snitch, they just use a LoadBalancingPolicy e.g. DCAwareRoundRobinPolicy, which combines both. They also listen for notifications through the binary protocol. I'm not saying this is the best way to do it, but is definitely a step in the right direction. Thanks for your help getting config extensions and topology discovery in there. Alex — Reply to this email directly or view it on GitHub.