Conversation
|
@fresh-borzoni Appreciate your review here 🙏 |
leekeiabstraction
force-pushed
the
cpp-integration-test
branch
from
fb9aaa9 to
21bab65
Compare
leekeiabstraction
force-pushed
the
cpp-integration-test
branch
from
21bab65 to
93a758f
Compare
|
@leekeiabstraction, it might be the case that we would need to get #351 merged first
|
|
That is fine. We can update this afterwards |
|
Still appreciate your early review on the other parts within this PR though! 🙏🏼 |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@leekeiabstraction Ty for the PR.
quickly looked through. Left couple of comments, but still 'review in progress'
PTAL
| } | ||
|
|
||
| // Wait for ZooKeeper to be ready before starting Fluss servers | ||
| std::this_thread::sleep_for(std::chrono::seconds(5)); |
There was a problem hiding this comment.
mb we should just poll for some readiness?
it's a bit more predictable, though I'm not sure what to chose as readiness marker, in rust we have .get_cluster().get_one_available_server() which seems specific to rust implementation only.
| inline std::vector<fluss::ScanRecord> PollRecords(fluss::LogScanner& scanner, | ||
| size_t expected_count, | ||
| int timeout_seconds = 10) { | ||
| // We need to keep the ScanRecords alive since ScanRecord borrows from them |
There was a problem hiding this comment.
we shall remove it as it just returns empty vector in the end.
But it actually made me think - mb we should just use shared_ptr and I overcomplicated this pattern, it basically would be Arc from Rust.
prepared the fix for this: seems to work and better overall: #353
|
|
||
| namespace fluss_test { | ||
|
|
||
| static constexpr const char* kFlussVersion = "0.7.0"; |
There was a problem hiding this comment.
Great question, it would, parameterising server version would be a good approach to take.
We'd want to parameterise this across different ITs, within website (for back referencing to main Fluss website) and anywhere else that might be applicable.
However I think we can address parameterising as an issue / separate PR.
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@leekeiabstraction finished review, left comments. PTAL
also once scan by buckets PR is merged - we need to add some tests for it
|
|
||
| // Create partial update writer to update only score column | ||
| auto partial_upsert = table.NewUpsert(); | ||
| partial_upsert.PartialUpdateByName({"id", "score"}); |
There was a problem hiding this comment.
Do we wish to test by Index as well?
| auto table_scan = scan_table.NewScan(); | ||
| fluss::LogScanner log_scanner; | ||
| ASSERT_OK(table_scan.CreateLogScanner(log_scanner)); | ||
| ASSERT_OK(log_scanner.Subscribe(0, 0)); |
There was a problem hiding this comment.
Why do we subscribe to single bucket? what if records would be distributed to different buckets?
We need to set bucket to 1 for the table at least.
the same pattern in other places, PTAL
2 participants
Purpose
Linked issue: close #338
Brief change log