-
Notifications
You must be signed in to change notification settings - Fork 28
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
Upgrade to C* Java Driver 4.7.2 #129
Conversation
@rustyrazorblade would you mind reviewing this? This fixes #127 |
Sorry, I can't help you here, I am no longer the maintainer. When I left TLP my commit access to this repo was removed. @adejanovski or @michaelsembwever might be able to help you though. |
Would you like me to restore commit access @rustyrazorblade? It was no one's intention for you to be removed, just the consequence of leaving the TLP org. |
A question I have with this PR is… won't clients (and benchmark runs) want to use different drivers? For those using version 4 of the driver this PR is important, but there is also plenty of users still using version 3. And for example it's not our first recommendation to upgrade from 3 to 4. (In most clusters and applications, there are often more pressing issues.) Ideally, won't we want to support both drivers? |
@michaelsembwever IMO we don't need to support multiple driver versions in tlp-stress (that would be a nightmare to support anyway). Clients of tlp-stress shouldn't care about which driver version is being internally used to produce load against their node/cluster. I would have thought that all a client would care about is that the requested load works against their C* version (driver 4.7.x supports C* 2.1 and newer) |
Thanks for the offer @michaelsembwever - unfortunately there's a couple other reasons that I won't get into right now that would still prevent me from working on this :( Since I no longer work on this as a community project I feel it would be inappropriate for me to merge people's code, and it would be a disservice to put less than my full attention and scrutiny into PRs and commits. I completely agree with @michaelsembwever about the different versions of the driver. I spent some time a ways back working on a branch to upgrade the driver: https://github.com/thelastpickle/tlp-stress/tree/jon/driver_upgrade Ultimately I abandoned it, because most folks weren't using the 4.0 version of the driver, and whether or not we like it there are driver differences when it comes to settings and performance. I lean towards using the community as a barometer, and if folks aren't using the 4.0 version of the driver I don't see much to gain from forcing it on them in the benchmarking tool. There are a number of settings that were a pain to change programmatically at the time - looks like some of that still is. It appears this patch just flat out deletes the ability to run in coordinator only mode: https://github.com/thelastpickle/tlp-stress/pull/129/files#diff-73f9c8bd5d6d825b31d4d7c217674d86R10, something I found valuable and was one of the reasons I abandoned my own patch. I'd just like to note - I'm only weighing in here as a former author to provide some context why things are the way they are. The burden of maintenance isn't on me, so I don't want to tell anyone what to do. As a user of tlp-stress I can say I wouldn't use it if the driver moved to 4.0 for the reasons I specified above unless it was optional as @michaelsembwever suggested. |
I believe eventually the community will switch to the 4.x line of the driver and that was my main motivator for doing this PR. Lots of things in the 4.x driver line changed within the last year and it got more mature/stable.
I agree that some things are a pain to configure, whereas other things are more clear and easier to configure. In terms of |
4.x will eventually become the popular use, but that's not yet today. Unless there is a way to make it configurable to switch between driver versions, then I would suggest this work stays in a maintained branch, e.g. something like |
@nastra thank you for putting the time and effort into creating and submitting this patch! It is good to see people care about the tool and are willing to help with it. I do agree with @michaelsembwever and @rustyrazorblade on where we go from here. I think the work you have done is useful, it just needs to be made configurable at runtime. Otherwise, it should stay on a branch until there is significantly more use of the 4.x driver. |
For reviewing, the official upgrade guide (https://github.com/datastax/java-driver/tree/4.x/upgrade_guide) can be used as a reference.
Test run here: https://app.circleci.com/pipelines/github/nastra/tlp-stress (not sure why it doesn't show up as "Checks" here)