Create Session Discovers Other Hosts #181
Conversation
…Hosts based on what the cluster has configured.
|
Is it possible to make this discovery optional (i.e. The client should specify explicitly that they want this behavior). Also, do you have an idea how you could test this? |
|
Yea I was thinking about making it optional. Adding that functionality right now. As far as testing, I'm not sure yet. Without mocking out a response, it seems it would take a lot of work. Currently, I just manually tested it with our stack. |
| @@ -60,6 +62,30 @@ func (cfg *ClusterConfig) CreateSession() (*Session, error) { | |||
| if pool.Size() > 0 { | |||
| s := NewSession(pool, *cfg) | |||
| s.SetConsistency(cfg.Consistency) | |||
|
|
|||
| if cfg.DiscoverHosts == true { | |||
There was a problem hiding this comment.
Since this value is a bool, it does not need an equality expression.
|
Yes, I can appreciate that this can be a difficult thing to test, especially given the fact that you need at least two different IP addresses in a test cluster. And then this needs to run as part of the CI build. So we're the gocql team is not a fanatical TDD outfit by any means, but we are looking to strike a balance between accepting valuable contributions and preventing potential regressions in the library. Basically I think that for cases that we can't tractably test, we'll need two maintainers to sign off on it. Speaking of CI builds, have you found out why the Travis build failed? |
|
I have not. Looking into this now. |
|
FWIW #179 is also afflicted by the problem of not being easy to test, despite introducing a reasonable change - I've started a thread on the discussion list to help us deal with this kind of thing. |
|
Sorry for being mia but I was following this and felt like I needed to voice an opinion. The whole purpose of the abstraction of connection pooling is to stop introducing functionality like this into the core. —Reply to this email directly or view it on GitHub. |
|
Well this is precisely the discussion I wanted to have. On it's own, this patch has merit, because it provides a simple approach to autodiscovery. But viewed in the context of the autodiscovery branch @phillipCouto has been working on, this patch comes across as just a point solution. That said, this patch is small and it is theoretically mergeable in its current state (modulo tests). The autodiscovery branch, OTOH, whilst being the more wholistic approach, appears to be some time off landing. So this leaves us in a dilemma - do we wait for the best solution or do we go for something that fills a need and is available now? |
|
Is it the aim then that every developer write their auto discovery connection pool? What is meant by the core and what is non-core? This is something that every cassandra developer would most likely benefit from and I really don't see what is gained by keeping it separate. An "autodiscovery connection pool" doesn't seem like that would be something terribly useful outside of cassandra. IMO, the driver should aim to match the datastax driver's features. |
|
Well I was use the auto discovery branch as points of reference for things that were done in it. I wouldn't merge the branch and again not against this functionality , I am just looking at what is core functionality and what is an extension to the core functionality. —Reply to this email directly or view it on GitHub. |
|
Exactly and auto discovery in datastax is not part of the core but in an auxiliary package that has other nice to haves that do not clutter the main package with all of the driver low level function. IMO, the driver should aim to match the datastax driver's features. —Reply to this email directly or view it on GitHub. |
|
In the Java driver? The Java driver as hosted at https://github.com/datastax/java-driver/tree/2.0/driver-core supports discovery, dc-aware load balancing, token awareness and configurable retry policies out of the box. While all of it is optional the driver does come "batteries included" with sane defaults (round robin is most likely good enough for a large number of deployments). You do have a point as they have made sure to logically separate the two layers so if you want to implement your own load balancing its possible - but I don't see why gocql shouldn't come batteries included. If most people are going to have 3 node deployments in the same datacenter the off-the-shelf driver should at least be aware of that. |
|
Haha sorry my memory served me wrong. Like I said I'm not opposed to it
|
|
Not going to have the time to write the test for this, as of now. Let me know though if you need help in this area to get this merged in. We'd really like this functionality. |
|
@phillipCouto How do we move forwards on this? This patch is not as comprehensive as the auto-discovery change you are proposing, but IMHO it is in a mergeable state now and hence I would be tempted to merge it. Another down side is that there is no regression tests for this, but realistically speaking, this would be very hard to do even with the auto-discovery branch. So given these two observations, I would go ahead with the merge. @benf-inova Unfortunately you've picked a change where the opinions are divided. I think your change is short and sweet and will probably work, but I do see Phillip's point of view, which is why I won't merge this unless we can reach an agreement. In addition, you would need to brush up a few things in the patch such as making the indentation consistent (not sure whether you ran go fmt), adding your name to the AUTHORs file and documenting this feature in the README. Furthermore, you should be aware of the fact that the absence of a test can potentially mean that a change in this area could break your app down the line. Obviously we'll try our best to maintain stability, but regressions can occur. |
|
What we can do is implement the change and then gauge the requests from the community. If it services our users and no request come in to enhance it then it can stay as is. If not we will spark ideas from our users to better the implementation and hone it in. As for the test I can work on writting a test to validate the functionality since I have some experience writting the test in the auto discovery branch. |
|
OK @phillipCouto, so you're down with this change for now (assuming that @benf-inova does the necessary tidy up and gets the build to pass)? |
|
@benf-inova I think the godoc for the
|
|
Changes are in, and TravisCI passed last time it ran. I assume it hasn't run again because no code was changed in the more recent commits? |
|
Yes I agree with the change with the cleans ups.
|
|
OK, I'd like to merge this, but right now it won't merge automatically. |
|
Looks like there is a merge conflict. Best thing would be for @benf-inova to merge the master back into his branch and resolve the conflict and commit the resolution. Or he can scrap this request and make a new one with less commits involved. |
|
Yes, that was my polite way of saying "If you want this merged, make it mergeable" :-) |
|
Should be good now. |
Create Session Discovers Other Hosts
|
Thank you very much for your contribution :-) |
fix(dealer): make net.Dialer not to bind to reuse ephimeral ports
When using Auto-scaling groups, it would be nice if the connection could discover the cluster for round-robin connections, rather than having to specify. This Pull Requests, adds this discovery when requesting a new session.