Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

nastra
Copy link

@nastra nastra commented Jul 8, 2020

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)

@nastra
Copy link
Author

nastra commented Jul 8, 2020

@rustyrazorblade would you mind reviewing this? This fixes #127

@rustyrazorblade
Copy link
Contributor

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.

@michaelsembwever
Copy link
Member

When I left TLP my commit access to this repo was removed.

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.

@michaelsembwever
Copy link
Member

michaelsembwever commented Jul 9, 2020

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?

@nastra
Copy link
Author

nastra commented Jul 9, 2020

@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)

@rustyrazorblade
Copy link
Contributor

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.

@nastra
Copy link
Author

nastra commented Jul 10, 2020

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.

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.

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 agree that some things are a pain to configure, whereas other things are more clear and easier to configure.

In terms of coordinator only mode my plan was not to delete that support. I wanted to get initial feedback for this PR and then fix SSL/DC Mode/Coordinator only mode. Does that sound reasonable?

@michaelsembwever
Copy link
Member

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.

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 master--java-driver-4.x

@ossarga
Copy link
Contributor

ossarga commented Jul 13, 2020

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.

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 master--java-driver-4.x

@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.

@nastra nastra closed this Apr 11, 2022
@nastra nastra deleted the upgrade-java-driver branch April 11, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants