-
Notifications
You must be signed in to change notification settings - Fork 253
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
[server] Cdc log visible only when the data already flush to rocksDb from preWriteBuffer #179
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add a test to reproduce the problem. Maybe add a test in ReplicaManagerTest
to verify the high-watermark of the given record is must advanced if the given record can be read in kv.
fluss-client/src/test/java/com/alibaba/fluss/client/admin/ClientToServerITCaseUtils.java
Outdated
Show resolved
Hide resolved
fluss-server/src/main/java/com/alibaba/fluss/server/replica/Replica.java
Outdated
Show resolved
Hide resolved
0cfd7e2
to
131d1a1
Compare
@wuchong comments addressed. |
fluss-server/src/main/java/com/alibaba/fluss/server/log/LogTablet.java
Outdated
Show resolved
Hide resolved
fluss-server/src/main/java/com/alibaba/fluss/server/replica/Replica.java
Outdated
Show resolved
Hide resolved
assertThat(lookups.get(0)).isNull(); | ||
replicaManager.putRecordsToKv( | ||
20000, | ||
1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test can reproduce the problem, because it only has one replia and ack=1, so after executing replicaManager.putRecordsToKv
, the watermark has been advanced, and kv data has been flushed, thus the following test is always success without any changes to the source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I switched to a multi-thread way to validate this issue. In the new test, if highWatermarkUpdateTimestamp
is less than lastTimestampForNullValue
even once, it indicates that the CDC log visibility is earlier than the KV. Specifically, highWatermarkUpdateTimestamp
is later than the actual highWatermarkUpdate
time, and lastTimestampForNullValue
is earlier than the collected time, which is intended to eliminate influence of collecting. Pr ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my local env, if I revert the change, the test will always failed.
1ff4fc6
to
2644ed5
Compare
@wuchong comments addressed. |
fluss-server/src/test/java/com/alibaba/fluss/server/replica/ReplicaManagerTest.java
Outdated
Show resolved
Hide resolved
…from preWriteBuffer
2644ed5
to
7d2b85a
Compare
Purpose
This pr is aims to fix the visibility of the CDC log, it should only become visible after the KV data has been flushed to RocksDb.
Linked issue: #134
Tests
API and Format
Documentation