caching_session: use custom hasher instead of ahash in tests #625
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Please rebase on top of latest |
piodul
force-pushed
the
drop-ahash-dev-dep
branch
from
119228a
to
4040880
Compare
|
@cvybhu rebased, ping |
avelanarius
approved these changes
Jan 17, 2023
piodul
added a commit
to piodul/scylla-rust-driver
that referenced
this pull request
Jan 18, 2023
Currently, we are importing the `ahash` crate for only one reason: to test that CachingSession works with custom hasher implementations. The custom hasher quality doesn't matter at all in this test, so it's very easy to write own implementation for testing purposes, rendering the dependency on `ahash` obsolete. The change was prompted by a report from one of our users which use a fork of the driver and managed to trigger a bug in cargo - most likely this one: scylladb#625 . The most likely cause was that the project specified a different, non-compatible version of ahash in the dependencies than the one in scylla's dev-dependencies. While it doesn't fix the cargo bug and it can still happen with other dev-dependencies, the bug should no longer happen with conflicting `ahash`. Moreover, removing an unneeded depdencency from a project is a good motivation in itself.
piodul
force-pushed
the
drop-ahash-dev-dep
branch
from
4040880
to
cb97feb
Compare
|
@avelanarius No, it was actually a bug in cargo's resolver which can prevent a dependent project from building at all. Added more info to the commit description. |
Currently, we are importing the `ahash` crate for only one reason: to test that CachingSession works with custom hasher implementations. The custom hasher quality doesn't matter at all in this test, so it's very easy to write own implementation for testing purposes, rendering the dependency on `ahash` obsolete. The change was prompted by a report from one of our users which use a fork of the driver and managed to trigger a bug in cargo - most likely this one: rust-lang/cargo#7463 . The most likely cause was that the project specified a different, non-compatible version of ahash in the dependencies than the one in scylla's dev-dependencies. While it doesn't fix the cargo bug and it can still happen with other dev-dependencies, the bug should no longer happen with conflicting `ahash`. Moreover, removing an unneeded depdencency from a project is a good motivation in itself.
piodul
force-pushed
the
drop-ahash-dev-dep
branch
from
cb97feb
to
51bcc2c
Compare
|
In the updated commit message:
Shouldn't the link lead somewhere else? |
Yes, I realized that and already pushed a commit with corrected message. |
|
@cvybhu ping |
cvybhu
approved these changes
Jan 21, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we are importing the
ahashcrate for only one reason: to test that CachingSession works with custom hasher implementations. The custom hasher quality doesn't matter at all in this test, so it's very easy to write own implementation for testing purposes, rendering the dependency onahashobsolete.The change was prompted by a report from one of our users which use a fork of the driver and managed to trigger a bug in cargo - most likely this one: rust-lang/cargo#7463 . The most likely cause was that the project specified a different, non-compatible version of ahash in the dependencies than the one in scylla's dev-dependencies. While it doesn't fix the cargo bug and it can still happen with other dev-dependencies, the bug should no longer happen with conflicting
ahash. Moreover, removing an unneeded depdencency from a project is a good motivation in itself.Pre-review checklist
I added relevant tests for new features and bug fixes.I added appropriateFixes:annotations to PR description.