Implement timestamp generators and enable 4 out of 5 TimestampTests #295
Conversation
This file contains hidden or 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
muzarski
added
the
P2
muzarski
added
area/testing
Lorak-mmk
approved these changes
May 6, 2025
Lorak-mmk
requested changes
May 6, 2025
wprzytula
requested changes
May 7, 2025
|
Rebased on |
|
v1.1: Addressed comments - most importantly fixed a bug where we would compare |
wprzytula
approved these changes
May 7, 2025
Lorak-mmk
approved these changes
May 7, 2025
Things to pay attention to during review: - warning threshold is passed in microseconds, while warning interval is passed in millis - documentation says that if threshold is <= 0, it should disable the warnings - documentation says that if interval is <= 0, it should fallback to 1ms interval
I was really curious why these methods were not defined in `testing_unimplemented`. The testing binary linking should fail. Well... I found out that they are defined in src/, where we still have a cpp implementation of the timestamp generators. Note: While having both cpp and rust implementations, the linker did not complain, and happily linked the test binary even though there are duplicate definitions of timestamp_gen methods. It seems that linker picked up the cpp implementations. Why? The test causes segfault when calling `cass_cluster_set_timestamp_gen` when we try to clone an Arc. It means that pointer `cass_timestamp_gen_server_side_new` does not originate from Arc allocation - this suggests that the origin of the pointer is different (created by cpp API).
The remaining test (MonotonicTimestampGenerator) needs further adjustments. Currently it segfaults - I'll create a separate issue for this one. TLDR - currently, the generator provided to `cass_cluster_set_timestamp_gen` is constructed using pure c++ API (no API from cassandra.h is used). It depends on `src/timestamp_generator.cpp`, which we should ultimately remove. However, to remove this file, we need to implement a testing utility which allows us to read the generated timestamps during the test. The test would then need to be rewritten using this new utility.
|
v1.3: Forgot to cover |
Lorak-mmk
approved these changes
May 7, 2025
wprzytula
approved these changes
May 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixes: #46
Ref: #132
This PR implements timestamp methods.
Enabled integration
TimestampTestsStatementBatchStatementServerSideTimestampGeneratorStatementServerSideTimestampGeneratorBatchStatementDisabled integration test
There is one test that we need to keep disabled (for now). I want to create a separate issue for this. issue: #296.
Pre-review checklist
[ ] I have implemented Rust unit tests for the features/changes introduced..github/workflows/build.ymlingtest_filter..github/workflows/cassandra.ymlingtest_filter.