KAFKA-806: Index may not always observe log.index.interval.bytes by FrankYang0529 · Pull Request #18012 · apache/kafka

FrankYang0529 · 2024-12-03T05:22:12Z

Currently, each log.append() will add at most 1 index entry, even when the appended data is larger than log.index.interval.bytes. One potential issue is that if a follower restarts after being down for a long time, it may fetch data much bigger than log.index.interval.bytes at a time. This means that fewer index entries are created, which can increase the fetch time from the consumers.

Changes in this PR:

Iterate each batch in MemoryRecords. If bytes since last index entry is large than log.index.interval.bytes, appending a offset/timestamp index.
In before, if a LogSegment is new, we set rollingBasedTimestamp as largest timestamp in the first MemoryRecords. After this PR, rollingBasedTimestamp is largest timestamp in the first batch, so the behavior is consistent between leader and follower.
Remove LogAppendInfor#shallowOffsetOfMaxTimestamp. We iterate each batch in append function, so we can get the value by shallowOffsetOfMaxTimestampSoFar.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

junrao

@FrankYang0529 : Thanks for the PR. Left a couple of comments.

junrao · 2025-01-07T22:36:42Z

+            for (RecordBatch batch : records.batches()) {
+                if (bytesSinceLastIndexEntry > indexIntervalBytes &&
+                    batch.lastOffset() >= offsetIndex().lastOffset()) {
+                    offsetIndex().append(batch.lastOffset(), physicalPosition);


Currently, every time we append an entry to the offset index, we also append an entry to the timestamp index. It would be useful to keep it that way.

Hi @junrao, thanks for review. I addressed both comments.

For timestamp, it's not always monotonic in records, so checking offset by timestamp index is not as much as offset index. Probably, we can consider whether it's worth to add timestamp for each batch, because this operation introduces more cost.

It's true that the time index is used less frequently. But I am not sure it's worth optimizing since its size is small.

Maybe we can do a bit refactor for this hot method to reduce the access of buffer and check.

for (RecordBatch batch : records.batches()) { var batchMaxTimestamp = batch.maxTimestamp(); var batchLastOffset = batch.lastOffset(); var updateTimeIndex = false; if (batchMaxTimestamp > maxTimestampSoFar()) { maxTimestampAndOffsetSoFar = new TimestampOffset(batchMaxTimestamp, batchLastOffset); updateTimeIndex = true; } if (bytesSinceLastIndexEntry > indexIntervalBytes) { offsetIndex().append(batchLastOffset, physicalPosition); // max timestamp may not be monotonic, so we need to check it to avoid the time index append error if (updateTimeIndex) timeIndex().maybeAppend(maxTimestampSoFar(), shallowOffsetOfMaxTimestampSoFar()); bytesSinceLastIndexEntry = 0; } var sizeInBytes = batch.sizeInBytes(); physicalPosition += sizeInBytes; bytesSinceLastIndexEntry += sizeInBytes; }

junrao · 2025-01-07T23:01:23Z

+            // append an entry to the offset index at batches level (if needed)
+            for (RecordBatch batch : records.batches()) {
+                if (bytesSinceLastIndexEntry > indexIntervalBytes &&
+                    batch.lastOffset() >= offsetIndex().lastOffset()) {


This is unnecessary. On the leader side, we call LogValidator.validateBatch, which makes sure each batch has at least one record.

junrao

@FrankYang0529 : Thanks for the updated PR. A few more comments.

junrao · 2025-01-08T23:01:52Z

+            long batchShallowOffsetOfMaxTimestamp = -1L;
+            // append an entry to the index at batches level (if needed)
+            for (RecordBatch batch : records.batches()) {
+                if (batch.maxTimestamp() > batchMaxTimestamp) {


Could we get rid of this condition and make batchMaxTimestamp and batchShallowOffsetOfMaxTimestamp a local val in the loop?

Yes, updated it.

junrao · 2025-01-08T23:02:55Z

            long appendedBytes = log.append(records);
            LOGGER.trace("Appended {} to {} at end offset {}", appendedBytes, log.file(), largestOffset);
-            // Update the in memory max timestamp and corresponding offset.
-            if (largestTimestampMs > maxTimestampSoFar()) {


With this change, we can remove both largestTimestampMs and shallowOffsetOfMaxTimestamp. We probably can also remove LogAppendInfo.shallowOffsetOfMaxTimestamp.

Yes, removed largestTimestampMs and shallowOffsetOfMaxTimestamp in LogSegment#append. Also, removed LogAppendInfo.shallowOffsetOfMaxTimestamp because it's not referenced anymore.

junrao · 2025-01-08T23:04:15Z

+                    offsetIndex().append(batch.lastOffset(), physicalPosition);
+
+                    // max timestamp may not be monotonic, so we need to check it to avoid the time index append error
+                    if (batchMaxTimestamp >= timeIndex().lastEntry().timestamp)


This is unnecessary since TimeIndex has the same check.

// We only append to the time index when the timestamp is greater than the last inserted timestamp. // If all the messages are in message format v0, the timestamp will always be NoTimestamp. In that case, the time // index will be empty. if (timestamp > lastEntry.timestamp) {

I think @FrankYang0529 tried to avoid the exception of adding a "smaller" timestamp. see

kafka/storage/src/main/java/org/apache/kafka/storage/internals/log/TimeIndex.java

Line 199 in c28d9a3

throw new IllegalStateException("Attempt to append a timestamp (" + timestamp + ") to slot " + entries()

Yes, like @chia7712 mentioned, if there is timestamp index in the file and new timestamp is less than old one, TimeIndex#maybeAppend will throw error.

junrao · 2025-01-08T23:09:13Z

+
+                    // max timestamp may not be monotonic, so we need to check it to avoid the time index append error
+                    if (batchMaxTimestamp >= timeIndex().lastEntry().timestamp)
+                        timeIndex().maybeAppend(batchMaxTimestamp, shallowOffsetOfMaxTimestampSoFar());


Hmm, we should use maxTimestampSoFar() instead batchMaxTimestamp, right?

Yes, updated it. Thanks.

junrao · 2025-01-08T23:22:16Z

+            for (RecordBatch batch : records.batches()) {
+                if (bytesSinceLastIndexEntry > indexIntervalBytes &&
+                    batch.lastOffset() >= offsetIndex().lastOffset()) {
+                    offsetIndex().append(batch.lastOffset(), physicalPosition);


It's true that the time index is used less frequently. But I am not sure it's worth optimizing since its size is small.

junrao

@FrankYang0529 : Thanks for the update PR. A few more comments.

junrao · 2025-01-09T17:57:30Z

            }
-            bytesSinceLastIndexEntry += records.sizeInBytes();
+
+            if (updateRollingBasedTimestamp) rollingBasedTimestamp = OptionalLong.of(recordsLargestTimestampMs);


It's better to assign the timestamp of the first batch in a segment to rollingBasedTimestamp. This way, it's consistent between the leader and the follower.

Since we utilize the timestamp of the first batch in other parts of the code, it is beneficial to maintain consistency by aligning it within this PR. Alternatively, we can keep rollingBasedTimestamp empty and initialize it during the method call.

Agree, for consistency, we can leave LogSegment#loadFirstBatchTimestamp to handle rollingBasedTimestamp. Remove related code in append function.

junrao · 2025-01-09T18:02:58Z

-    new LogAppendInfo(firstOffset, lastOffset, lastLeaderEpochOpt, maxTimestamp, shallowOffsetOfMaxTimestamp,
-      RecordBatch.NO_TIMESTAMP, logStartOffset, RecordValidationStats.EMPTY, sourceCompression,
-      validBytesCount, lastOffsetOfFirstBatch, Collections.emptyList[RecordError], LeaderHwChange.NONE)
+    new LogAppendInfo(firstOffset, lastOffset, lastLeaderEpochOpt, maxTimestamp, RecordBatch.NO_TIMESTAMP, logStartOffset, RecordValidationStats.EMPTY, sourceCompression, validBytesCount, lastOffsetOfFirstBatch, Collections.emptyList[RecordError], LeaderHwChange.NONE)


It's probably better to still split this long statement into multiple lines.

Yes, updated it. Thanks.

junrao · 2025-01-09T18:31:17Z


-            seg.append(41, RecordBatch.NO_TIMESTAMP, -1L, v1Records(40, "hello", "there"));
+            seg.append(41,
+                MemoryRecords.withRecords(RecordBatch.MAGIC_VALUE_V1, 40, Compression.NONE, TimestampType.CREATE_TIME,


Why is this change needed? Also, since we no longer support v0/v1 message format. We need to remove all v1Records usage in this test. Should we do it in this PR or a separate one?

In 4.0, we don't support v0/v1 message from producer. However, do we support a case that a 4.0 broker works with other versions and the leader node has old message format? In this case, we may still get v0/v1 message format.

I think @FrankYang0529 is interested in testing a scenario where a 4.0 follower sends a FetchRequest to a leader that contains v0/v1 messages.

Ok. Why do we change from inserting 2 records to 1 record? Also, could we continue using v1Records?

Why do we change from inserting 2 records to 1 record?

My mistake. Changed it back to 2 records.

could we continue using v1Records?

Probably no. The old test rely on largestTimestampMs to assign rollingBasedTimestamp as RecordBatch.NO_TIMESTAMP. In this PR, we assign first batch max timestamp for rollingBasedTimestamp. In v1Records, it sets record timestamp as offset * 10, so it's not RecordBatch.NO_TIMESTAMP.

kafka/storage/src/test/java/org/apache/kafka/storage/internals/log/LogSegmentTest.java

Line 345 in da0c3be

seg.append(41, RecordBatch.NO_TIMESTAMP, -1L, v1Records(40, "hello", "there"));

kafka/storage/src/test/java/org/apache/kafka/storage/internals/log/LogSegmentTest.java

Lines 95 to 98 in da0c3be

private MemoryRecords v1Records(long offset, String... records) {

List<SimpleRecord> simpleRecords = new ArrayList<>();

for (String s : records) {

simpleRecords.add(new SimpleRecord(offset * 10, s.getBytes()));

junrao · 2025-01-09T19:18:51Z

+                boolean updateTimeIndex = false;
+                if (batchMaxTimestamp > maxTimestampSoFar()) {
+                    maxTimestampAndOffsetSoFar = new TimestampOffset(batchMaxTimestamp, batchLastOffset);
+                    updateTimeIndex = true;


This code seems still not quite right. We need to remember updateTimeIndex once we get a higher timestamp instead of resetting it in every batch.

Yes, remove updateTimeIndex. Thanks.

junrao · 2025-01-09T19:21:17Z

+                    offsetIndex().append(batchLastOffset, physicalPosition);
+
+                    // max timestamp may not be monotonic, so we need to check it to avoid the time index append error
+                    if (updateTimeIndex) timeIndex().maybeAppend(maxTimestampSoFar(), shallowOffsetOfMaxTimestampSoFar());


It seems that we don't need to maintain updateTimeIndex. maxTimestampSoFar() is always >= the last time index entry. If it's equal, it will be ignored in timeIndex().maybeAppend and won't trigger IllegalStateException.

The purpose of using updateTimeIndex is to minimize unnecessary calls to maybeAppend by ensuring that it's only invoked when a larger timestamp is encountered. That can avoid requiring unnecessary locking, but I think maybe that is unnecessary optimization. We can totally remove updateTimeIndex to simplify code

junrao · 2025-01-09T19:27:07Z

                expectedMaxTimestampOffset = 2;
                break;
        }
-        assertEquals(expectedMaxTimestampOffset, validatedResults.shallowOffsetOfMaxTimestamp);


expectedMaxTimestampOffset is no longer used and should be removed.

Removed it. Thanks.

junrao · 2025-01-09T19:33:12Z

+        assertEquals(1, segment.offsetIndex().lookup(1L).offset);
+        assertEquals(2, segment.offsetIndex().lookup(2L).offset);
+
+        assertEquals(1, segment.timeIndex().entries());


It seems that we should have two index entries in this test, the first for timestamp 1 and the second of timestamp 2.

Yes, updated it.

junrao · 2025-01-09T19:34:46Z

+
+        assertEquals(1, segment.timeIndex().entries());
+        assertEquals(0L, segment.timeIndex().lookup(1L).offset);
+        assertEquals(2L, segment.timeIndex().lookup(2L).offset);


Could we use timeIndex().entry to explicitly fetch each index entry instead of using timeIndex().lookup?

Updated assertion. Thanks.

junrao

@FrankYang0529 : Thanks for the updated PR. A few more comments.

junrao · 2025-01-14T00:43:40Z

+                if (bytesSinceLastIndexEntry > indexIntervalBytes) {
+                    offsetIndex().append(batchLastOffset, physicalPosition);
+                    timeIndex().maybeAppend(maxTimestampSoFar(), shallowOffsetOfMaxTimestampSoFar());
+


extra newline

junrao · 2025-01-14T01:09:21Z

+        assertEquals(2, segment.offsetIndex().lookup(2L).offset);
+
+        assertEquals(2, segment.timeIndex().entries());
+        assertEquals(new TimestampOffset(1, 0), segment.timeIndex().entry(0));


Could we use entry for the offset index and the other test too?

Thanks for the suggestion. I only check offset field for offset index, because the position field may looks like some kind of magic value. WDYT?

junrao · 2025-01-14T01:15:38Z


-            seg.append(41, RecordBatch.NO_TIMESTAMP, -1L, v1Records(40, "hello", "there"));
+            seg.append(41,
+                MemoryRecords.withRecords(RecordBatch.MAGIC_VALUE_V1, 40, Compression.NONE, TimestampType.CREATE_TIME,


Ok. Why do we change from inserting 2 records to 1 record? Also, could we continue using v1Records?

junrao

@FrankYang0529 : Thanks for the updated PR. LGTM. Are the flaky tests related to this PR?

FrankYang0529 · 2025-01-15T08:05:09Z

Hi @junrao, thanks for the review. I check failed case and all are not related to this PR. I can't reproduce them on my laptop.

RemoteIndexCacheTest#testCleanerThreadShutdown: https://issues.apache.org/jira/browse/KAFKA-18537
AbstractCoordinatorTest#testWakeupAfterSyncGroupReceivedExternalCompletion: https://issues.apache.org/jira/browse/KAFKA-18310
DynamicConnectionQuotaTest#testDynamicConnectionQuota: https://issues.apache.org/jira/browse/KAFKA-8059

FrankYang0529 · 2025-01-15T13:01:27Z

Last build: https://github.com/apache/kafka/actions/runs/12784403901/job/35637865171?pr=18012

ClientUtilsTest#testParseAndValidateAddressesWithReverseLookup: this will be fixed in HOTFIX: ClientUtilsTest#testParseAndValidateAddressesWithReverseLookup #18549
AbstractCoordinatorTest#testWakeupAfterSyncGroupReceivedExternalCompletion: https://issues.apache.org/jira/browse/KAFKA-18310
KafkaStreamsTelemetryIntegrationTest#shouldPassCorrectMetricsDynamicInstances: https://issues.apache.org/jira/browse/KAFKA-18541

chia7712

@FrankYang0529 thanks for this patch. please take a look at following two minor questions

chia7712 · 2025-01-15T16:31:34Z

            now,
            records,
            maxTimestamp,
-            shallowOffsetOfMaxTimestamp,


shallowOffsetOfMaxTimestamp is useless, so could you please remove it also?

Removed it. Thanks for the reminder.

chia7712 · 2025-01-15T16:40:45Z

+
+                if (bytesSinceLastIndexEntry > indexIntervalBytes) {
+                    offsetIndex().append(batchLastOffset, physicalPosition);
+                    timeIndex().maybeAppend(maxTimestampSoFar(), shallowOffsetOfMaxTimestampSoFar());


If two batches have the same timestamp, and both timestamps are greater than maxTimestampSoFar, and the first batch is skipped due to indexIntervalBytes, the resulting index for searching the 'earliest offset' of the maximum timestamp using TimestampSpec will be incorrect.

Hmm, in that case, maxTimestampAndOffsetSoFar will point to the offset of the first batch, which will be appended to the time index. This seem correct, right?

you are right, we update the maxTimestampAndOffsetSoFar only if batchMaxTimestamp > maxTimestampSoFar() is true. Hence, the offset point to the first batch.

Signed-off-by: PoAn Yang <payang@apache.org>

junrao

@FrankYang0529 : Thanks for the updated PR. One more minor comment.

junrao · 2025-01-16T17:30:14Z

                                                       MetricsRecorder metricsRecorder) {
        long now = time.milliseconds();
        long maxTimestamp = RecordBatch.NO_TIMESTAMP;
-        long shallowOffsetOfMaxTimestamp = -1L;


Could we also remove initialOffset?

Signed-off-by: PoAn Yang <payang@apache.org>

junrao

@FrankYang0529 : Thanks for the updated PR. LGTM

ijuma · 2025-01-17T20:28:44Z

Can we please update the PR description with what we decided to do here? It's a bit sparse.

FrankYang0529 · 2025-01-18T01:53:47Z

@junrao @chia7712, thanks for the review.

@ijuma, updated the PR description. Thanks.

ijuma · 2025-01-18T02:13:30Z

Thanks! The last bullet point seems to be missing a word or something.

…che#18012) Currently, each log.append() will add at most 1 index entry, even when the appended data is larger than log.index.interval.bytes. One potential issue is that if a follower restarts after being down for a long time, it may fetch data much bigger than log.index.interval.bytes at a time. This means that fewer index entries are created, which can increase the fetch time from the consumers. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Jun Rao <junrao@gmail.com>

chia7712 · 2025-02-06T22:14:59Z

-            }
-            // append an entry to the index (if needed)
-            if (bytesSinceLastIndexEntry > indexIntervalBytes) {
-                offsetIndex().append(largestOffset, physicalPosition);


In scenarios where a new follower is synchronizing from another replica, another issue arises. If the records consists of multiple batches, the physicalPosition will reflect the position of the first batch. This leads to an inaccurate index, represented as (offset_of_last_batch, position_of_first_batch). This issue can be easily reproduced, and our dump log tool currently displays warnings to indicate this inconsistency.

Mismatches in :/home/chia7712/ikea-0-follower/00000000000000000000.index Index offset: 2062680, log offset: 2061012 Index offset: 2060984, log offset: 2058974 Index offset: 2058940, log offset: 2056966 Index offset: 2056936, log offset: 2054979 Index offset: 2054950, log offset: 2052962 Index offset: 2052931, log offset: 2050959 Index offset: 2050923, log offset: 2048924 Index offset: 2048895, log offset: 2046967

This drawback is analogous to having fewer index entries, which can lead to increased fetch times. I'm happy that @FrankYang0529 fix "two" issues by this PR.

However, I'm wondering if we should address this issue in the active 3.x version. We could calculate the accurate position by using the size of the last batch. For example:

int position = records.lastBatch().map(b -> log.sizeInBytes() - b.sizeInBytes()).orElse(physicalPosition); offsetIndex().append(largestOffset, position);

@chia7712 Thanks for the information. I will create a PR to 3.9 tomorrow.

@FrankYang0529 I open https://issues.apache.org/jira/browse/KAFKA-18759 to trace it for 3.9 and 3.8 - please feel free to take over this jira

@chia7712 : If we want to address the indexing issue in 3.9, it's probably easier to just cherry-pick this PR to 3.9 instead of fixing a subset of the problem separately, right?

@junrao I tried to minimize the changes to the 3.9, but you are right that we can backport this PR instead.

@FrankYang0529 WDYT? If you agree to backport this PR, could you please file a PR for 3.9? Thanks.

4.0: #18842
3.9: #18843

…che#18012) Currently, each log.append() will add at most 1 index entry, even when the appended data is larger than log.index.interval.bytes. One potential issue is that if a follower restarts after being down for a long time, it may fetch data much bigger than log.index.interval.bytes at a time. This means that fewer index entries are created, which can increase the fetch time from the consumers. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Jun Rao <junrao@gmail.com> (cherry picked from commit e124d39)

…che#18012) Currently, each log.append() will add at most 1 index entry, even when the appended data is larger than log.index.interval.bytes. One potential issue is that if a follower restarts after being down for a long time, it may fetch data much bigger than log.index.interval.bytes at a time. This means that fewer index entries are created, which can increase the fetch time from the consumers. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Jun Rao <junrao@gmail.com>

…che#18012) Currently, each log.append() will add at most 1 index entry, even when the appended data is larger than log.index.interval.bytes. One potential issue is that if a follower restarts after being down for a long time, it may fetch data much bigger than log.index.interval.bytes at a time. This means that fewer index entries are created, which can increase the fetch time from the consumers. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Jun Rao <junrao@gmail.com> (cherry picked from commit e124d39)

github-actions Bot added storage Pull requests that target the storage module small Small PRs labels Dec 3, 2024

FrankYang0529 force-pushed the KAFKA-806 branch 2 times, most recently from d21b5f6 to e8d2320 Compare December 3, 2024 11:50

FrankYang0529 requested a review from chia7712 December 3, 2024 14:16

FrankYang0529 force-pushed the KAFKA-806 branch 2 times, most recently from a80cc4c to ebfb248 Compare December 7, 2024 05:41

ijuma requested a review from junrao December 24, 2024 15:40

junrao reviewed Jan 7, 2025

View reviewed changes

FrankYang0529 force-pushed the KAFKA-806 branch from ebfb248 to dbed266 Compare January 8, 2025 14:41

github-actions Bot removed the small Small PRs label Jan 8, 2025

FrankYang0529 force-pushed the KAFKA-806 branch 2 times, most recently from b9f1ae1 to e08001e Compare January 8, 2025 14:58

junrao reviewed Jan 8, 2025

View reviewed changes

FrankYang0529 force-pushed the KAFKA-806 branch from e08001e to fd8bbc9 Compare January 9, 2025 10:37

github-actions Bot added the core Kafka Broker label Jan 9, 2025

junrao reviewed Jan 9, 2025

View reviewed changes

FrankYang0529 force-pushed the KAFKA-806 branch from fd8bbc9 to 80c9ad4 Compare January 13, 2025 12:42

junrao reviewed Jan 14, 2025

View reviewed changes

FrankYang0529 force-pushed the KAFKA-806 branch 3 times, most recently from bed1b2b to 717f252 Compare January 14, 2025 05:01

junrao approved these changes Jan 14, 2025

View reviewed changes

FrankYang0529 force-pushed the KAFKA-806 branch from 717f252 to 57036cf Compare January 15, 2025 08:32

chia7712 reviewed Jan 15, 2025

View reviewed changes

FrankYang0529 added 3 commits January 16, 2025 10:27

KAFKA-806: Index may not always observe log.index.interval.bytes

962a2f9

Signed-off-by: PoAn Yang <payang@apache.org>

address comment

6180c23

Signed-off-by: PoAn Yang <payang@apache.org>

address comment

30b8b4c

Signed-off-by: PoAn Yang <payang@apache.org>

FrankYang0529 added 4 commits January 16, 2025 10:27

remove largestTimestampMs and shallowOffsetOfMaxTimestamp

10821de

Signed-off-by: PoAn Yang <payang@apache.org>

address comment

937c2be

Signed-off-by: PoAn Yang <payang@apache.org>

address comment

6bc0b4c

Signed-off-by: PoAn Yang <payang@apache.org>

address comment

7b20d92

Signed-off-by: PoAn Yang <payang@apache.org>

FrankYang0529 force-pushed the KAFKA-806 branch from 57036cf to 7b20d92 Compare January 16, 2025 02:32

FrankYang0529 requested a review from chia7712 January 16, 2025 06:33

junrao reviewed Jan 16, 2025

View reviewed changes

FrankYang0529 added 2 commits January 17, 2025 09:15

Merge branch 'trunk' into KAFKA-806

7d2d4cb

address comment

2765e38

Signed-off-by: PoAn Yang <payang@apache.org>

junrao approved these changes Jan 17, 2025

View reviewed changes

junrao merged commit e124d39 into apache:trunk Jan 17, 2025

FrankYang0529 deleted the KAFKA-806 branch January 18, 2025 01:53

chia7712 reviewed Feb 6, 2025

View reviewed changes

	private MemoryRecords v1Records(long offset, String... records) {
	List<SimpleRecord> simpleRecords = new ArrayList<>();
	for (String s : records) {
	simpleRecords.add(new SimpleRecord(offset * 10, s.getBytes()));

Conversation

FrankYang0529 commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrankYang0529 commented Dec 3, 2024 •

edited

Loading

chia7712 Jan 13, 2025 •

edited

Loading