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

CASSGO-21 Remove HostPoolHostPolicy from gocql package #1770

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

tengu-alt
Copy link

@tengu-alt tengu-alt commented Jun 11, 2024

According to issue #1517 HostPoolHostPolicy was moved to a separate package, and for now, users don't need to download dependency if they aren't using it.

@joao-r-reis
Copy link
Contributor

Maybe we can shorten the package name a bit to hostpoolpolicy?

@joao-r-reis joao-r-reis changed the title HostPoolHostPolicy was moved to separate package CASSGO-21 HostPoolHostPolicy was moved to separate package Oct 30, 2024
@tengu-alt
Copy link
Author

Maybe we can shorten the package name a bit to hostpoolpolicy?

I would prefer to keep the HostPoolHostPolicy because that provides more consistent naming.

@martin-sucha
Copy link
Contributor

  • Does anybody actually use this policy?
  • Does having it in a separate package (as opposed to a separate module / repository) prevent downloading the dependency?

@martin-sucha
Copy link
Contributor

For my first question, a search on Github finds a few uses.

@joao-r-reis
Copy link
Contributor

Does having it in a separate package (as opposed to a separate module / repository) prevent downloading the dependency?

I tested this locally and go mod tidy would add a github.com/hailocab/go-hostpool //indirect line to go.mod on my test project even if I didn't use the policy and when I switched to this branch (using replace) and ran go mod tidy again, then the github.com/hailocab/go-hostpool //indirect would disappear from go.mod. Idk if this is enough to test whether the dependency is being downloaded or not though

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Nov 1, 2024

@martin-sucha I noticed that for the lz4 dependency the approach that was taken was to create its own independent module, if this approach of having it on a separate package is enough should we remove the lz4 module and make it a package of the "main" module instead?

This wouldn't be addressed in this PR but maybe the 2 things (hostpoolhostpolicy and lz4) should follow the same approach for consistency.

@ribaraka
Copy link

ribaraka commented Nov 11, 2024

I tested this locally and go mod tidy would add a github.com/hailocab/go-hostpool //indirect line to go.mod on my test project even if I didn't use the policy and when I switched to this branch (using replace) and ran go mod tidy again, then the github.com/hailocab/go-hostpool //indirect would disappear from go.mod.

Double-checked it with @tengu-alt: after switching to the tengu-alt:move-hostpool-hostpolicy branch, the dependencies are disappearing, so having it in a separate package (as opposed to a separate module / repository) prevent downloading the dependency.

@ribaraka
Copy link

I noticed that for the lz4 dependency the approach that was taken was to create its own independent module, if this approach of having it on a separate package is enough should we remove the lz4 module and make it a package of the "main" module instead?

After encountering an issue with CI and refactoring gocql/lz4, I see little justification for maintaining LZ4 as an independent go module.

The gocql/lz4 module is specifically built to implement the gocql.Compressor interface, making it primarily suited for gocql. Supporting a separate LZ4 implementation beyond of gocql doesn’t add much value and might introduce unnecessary dependencies, increasing both the size and complexity of projects that rely on gocql. If it remains independent, it would need to be maintained in alignment with potential external projects, adding to its maintenance complexity without clear benefit to the core gocql project.

Given that I think, merging lz4 into gocql seems more practical. The merge would simplify dependency management for gocql users, reduce the module count, and make the overall structure easier to manage, versioning it independently is unnecessary, eliminate the need for users to manage multiple dependencies and avoid the potential for version incompatibility between gocql and gocql/lz4. A single, unified gocql module would be more user-friendly and streamlined.

@ribaraka
Copy link

Following the recent adjustments with LZ4 and the removal of unnecessary dependencies, I see an additional enhancement opportunity: moving the snappy into a separate package. This way, it won’t be downloaded when users only require LZ4.

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Nov 12, 2024

+1 on using packages instead of separate modules for lz4, snappy, and hostpoolhostpolicy. I brought this up on the C* dev Mailing List and slack but didn't get any feedback on it. I think we can proceed with this approach though.

@joao-r-reis
Copy link
Contributor

Please create a JIRA to move snappy to a separate package and a JIRA to move lz4 to a separate package. Let's have them target 2.0.0 as well.

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Nov 12, 2024

I would prefer to keep the HostPoolHostPolicy because that provides more consistent naming.

One reason why I think hostpool would be more appropriate is that we're creating this separate package for code that uses the hostpool dependency. Let's say we implement something else for gocql that uses this dependency but it's not a hostpolicy, would we add this component to this package or add a new package for it? It would make more sense to add an hypothetical new component that relies on hostpool to the existing hostpoolhostpolicy package but now the name of the package wouldn't make sense anymore because it's not only about a hostpolicy.

hostpool would also be more consistent with other packages like snappy and lz4, instead of snappycompressor and lz4compressor

@tengu-alt
Copy link
Author

I would prefer to keep the HostPoolHostPolicy because that provides more consistent naming.

One reason why I think hostpool would be more appropriate is that we're creating this separate package for code that uses the hostpool dependency. Let's say we implement something else for gocql that uses this dependency but it's not a hostpolicy, would we add this component to this package or add a new package for it? It would make more sense to add an hypothetical new component that relies on hostpool to the existing hostpoolhostpolicy package but now the name of the package wouldn't make sense anymore because it's not only about a hostpolicy.

hostpool would also be more consistent with other packages like snappy and lz4, instead of snappycompressor and lz4compressor

I see, that makes sense, I will change it.

@worryg0d
Copy link

Please create a JIRA to move snappy to a separate package and a JIRA to move lz4 to a separate package. Let's have them target 2.0.0 as well.

@joao-r-reis so I can leave the replace approach to run integration tests with lz4 over #1822?

@joao-r-reis
Copy link
Contributor

Please create a JIRA to move snappy to a separate package and a JIRA to move lz4 to a separate package. Let's have them target 2.0.0 as well.

@joao-r-reis so I can leave the replace approach to run integration tests with lz4 over #1822?

Yes. And snappy will also go to its own separate JIRA and PR, this one is only for hostpool.

@joao-r-reis
Copy link
Contributor

I think we should have the package directory at the root instead of a /pkg folder, so:

- files for gocql package at the root directory
- /hostpool
  - files for hostpool package
- /lz4
  - files for lz4 package
- /snappy
  - files for snappy package
- /internal
  - /internal/ccm
  - /internal/murmur
  - other internal packages

Maybe in the future we can also move the gocql package files into a gocql directory but that's a topic for another time.

HostPoolHostPolicy was moved to a separate package, and users don't need to download dependency if they aren't using it.

patch by Oleksandr Luzhniy; reviewed by João Reis, Stanislav Bychkov, for CASSGO-21
@tengu-alt tengu-alt changed the title CASSGO-21 HostPoolHostPolicy was moved to separate package CASSGO-21 Remove HostPoolHostPolicy from gocql package Nov 18, 2024
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.

5 participants