Flink: Empty implementation for pauseOrResumeSplits to prevent UnsupportedOperationException #9308

pvary · 2023-12-15T15:23:19Z

Based on the discussion with @mas-chen and @stevenzwu, there is no need to implement split level watermark alignment for Iceberg source, because the IcebergSourceSplitReader only reads splits sequentially. When waiting for watermark alignment the SourceOperator will stop processing and recycling the fetched batches. This exhausts the {@link ArrayPoolDataIteratorBatcher#pool} and the currentReader.next() call will be blocked even without split-level watermark alignment. Based on this the pauseOrResumeSplits and the wakeUp are left empty.

pvary · 2023-12-15T15:24:01Z

CC: @ajantha-bhat - further fixes on the test

rodmeneses · 2023-12-15T17:22:13Z

flink/v1.18/build.gradle

@@ -63,6 +63,7 @@ project(":iceberg-flink:iceberg-flink-${flinkMajorVersion}") {
      // These artifacts are shaded and included in the orc-core fat jar
      exclude group: 'com.google.protobuf', module: 'protobuf-java'
      exclude group: 'org.apache.hive', module: 'hive-storage-api'
+      exclude group: 'org.slf4j'


out of curiosity: why was this needed?

We specify 2.x slf4j as an Iceberg dependency. But these dependencies carry a 1.x slf4j transitive dependency. When both 1.x and 2.x slf4j is on classpath the the loggers are not working, so it is very hard to follow what happens during the tests

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

...flink/src/test/java/org/apache/iceberg/flink/source/reader/TestIcebergSourceSplitReader.java

rodmeneses

some minor comments, otherwise LGTM

stevenzwu · 2023-12-15T18:58:07Z

@pvary is the whole PR addressing the flaky test? if not, please separate out the flaky test part (hopefully small) into a separate PR. I would like to merge the flaky test fix ASAP. And you can include all 3 versions (1.16, 1.17, 1.18) in one PR.

pvary · 2023-12-15T19:54:12Z

@pvary is the whole PR addressing the flaky test? if not, please separate out the flaky test part (hopefully small) into a separate PR. I would like to merge the flaky test fix ASAP. And you can include all 3 versions (1.16, 1.17, 1.18) in one PR.

As mentioned in the description, the missing pauseOrResumeSplits method causes flakiness. I think it is better to fix it in one PR, than having a PR disabling the test, and another PR adding the test back. WDYT?

stevenzwu

@pvary we are changing some critical code here. I would prefer we take a little more time here and get more people to review.

Hence I would prefer if we disable/ignore the test first so that we make other PR/CI builds stable ASAP.

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

...k/src/test/java/org/apache/iceberg/flink/source/TestIcebergSourceWithWatermarkExtractor.java

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

pvary · 2023-12-17T05:13:02Z

@pvary we are changing some critical code here. I would prefer we take a little more time here and get more people to review.

Hence I would prefer if we disable/ignore the test first so that we make other PR/CI builds stable ASAP.

That is why I created #9309. That contains the most important other fixes.

pvary · 2023-12-17T11:45:12Z

@stevenzwu: Here is what I have found:
wakeUp is called whenever there is a new message concerning the SplitReader:

new split added/removed
split paused/resumed

These messages are executed in the same thread as the fetch method, so in this case we have to return from the fetch loop, even with empty results. This is somewhat concerning to me considering the comment here:

iceberg/flink/v1.18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

Lines 76 to 78 in 820fc3c

    
           // return an empty result, which will lead to split fetch to be idle. 
        
           // SplitFetcherManager will then close idle fetcher. 
        
           return new RecordsBySplits(Collections.emptyMap(), Collections.emptySet());

I do not see that it is closed, but probably you know more...

pvary · 2023-12-18T12:41:57Z

These messages are executed in the same thread as the fetch method, so in this case we have to return from the fetch loop, even with empty results. This is somewhat concerning to me considering the comment here:

I have checked, and the idleness is defined like this:
https://github.com/apache/flink/blob/1d33773e6b7f9f76f03ff8ffd73171b95fa24ccb/flink-connectors/flink-connector-base/src/main/java/org/apache/flink/connector/base/source/reader/fetcher/SplitFetcher.java#L338-L345

    boolean isIdle() {
        lock.lock();
        try {
            return assignedSplits.isEmpty() && taskQueue.isEmpty() && runningTask == null;
        } finally {
            lock.unlock();
        }
    }

So if we have tasks (split manipulation/pause), then we are not idle, so the Fetcher is not removed.

Also the Fetcher also handles the wakeUp flag, so we are golden:
https://github.com/apache/flink/blob/c28dd942e3dded4cab7a33037077a1d3bfa0929e/flink-connectors/flink-connector-base/src/main/java/org/apache/flink/connector/base/source/reader/fetcher/FetchTask.java#L61-L71

            if (!isWakenUp()) {
                // The order matters here. We must first put the last records into the queue.
                // This ensures the handling of the fetched records is atomic to wakeup.
                if (elementsQueue.put(fetcherIndex, lastRecords)) {
                    if (!lastRecords.finishedSplits().isEmpty()) {
                        // The callback does not throw InterruptedException.
                        splitFinishedCallback.accept(lastRecords.finishedSplits());
                    }
                    lastRecords = null;
                }
            }

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

mas-chen · 2023-12-18T18:08:28Z

...flink/src/test/java/org/apache/iceberg/flink/source/reader/TestIcebergSourceSplitReader.java

+    // Unpause the reading of the split, and check the result size
+    reader.pauseOrResumeSplits(ImmutableList.of(), ImmutableList.of(split));
+    result = (ArrayBatchRecords) reader.fetch();
+    assertThat(result.numberOfRecords()).isEqualTo(3);


Suggested change

assertThat(result.numberOfRecords()).isEqualTo(3);

assertThat(result.numberOfRecords()).isGreaterThan(2);

For test stability, I think fetch may not be guaranteed to return 1 record after pausing.

The reader reads the records in batches. The size of the batches are governed by:

public static final ConfigOption<Integer> SOURCE_READER_FETCH_BATCH_RECORD_COUNT = ConfigOptions.key("table.exec.iceberg.fetch-batch-record-count") .intType() .defaultValue(2048) .withDescription("The target number of records for Iceberg reader fetch batch.");

The default is 2048, so all of the records are read here

...flink/src/test/java/org/apache/iceberg/flink/source/reader/TestIcebergSourceSplitReader.java

mas-chen · 2023-12-18T18:11:18Z

...flink/src/test/java/org/apache/iceberg/flink/source/reader/TestIcebergSourceSplitReader.java

+    // Unpause and read the data in th next fetch
+    reader.pauseOrResumeSplits(ImmutableList.of(), ImmutableList.of(split));
+    result = (ArrayBatchRecords) reader.fetch();
+    assertThat(result.numberOfRecords()).isEqualTo(3);


ditto same comment here

We will get all 3 records

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

mas-chen · 2023-12-18T18:21:21Z

BTW, should we add an integration test?

pvary · 2023-12-18T19:03:22Z

BTW, should we add an integration test?

That reminds me, to remove the set(PipelineOptions.ALLOW_UNALIGNED_SOURCE_SPLITS, true) from the TestIcebergSourceWithWatermarkExtractor test. That is how I have found this issue.

The TestIcebergSourceWithWatermarkExtractor.testThrottling was slightly flaky because of this change (1 out of 1000), but I do not see any other good way to integration test it.

pvary · 2023-12-18T19:04:43Z

...k/src/test/java/org/apache/iceberg/flink/source/TestIcebergSourceWithWatermarkExtractor.java

@@ -383,8 +381,10 @@ protected IcebergSource<RowData> source() {
        .project(TestFixtures.TS_SCHEMA)
        .splitSize(100L)
        .streaming(true)
-        .monitorInterval(Duration.ofMillis(2))
+        .monitorInterval(Duration.ofMillis(100))


Added this, because the 2ms will causing too much load and ended up causing flakiness:

[collect-sink-operator-coordinator-executor-thread-pool-thread-1] WARN org.apache.flink.streaming.api.operators.collect.CollectSinkOperatorCoordinator - Collect sink coordinator encounters a CompletionException: java.net.SocketTimeoutException: Read timed out

2ms may cause high cpu usage. but why would it cause flakiness?

let's change it to 10 ms then. smaller value can discover new files sooner and make the test run faster. we already have a lot of integration tests using 10 ms and they are running fine.

I run the tests during the whole day yesterday, and found that the previous fix removed most of the flakiness, but we still have some issues which fails around 1000-2000 runs.

In failure cases the records are emitted by the emitter, but none of the are arriving to the Sink. I see the above error in the logs in this case: SocketTimeout on the sink coordinator, and retries - this appears multiple times before the timeout. Also the log is swamped with the planner logs.

With increasing the monitoring interval, I had 3500 successful runs in a row, when I decided that I need to spend my time elsewhere. As you can imagine this is nothing conclusive, and given the failure rate, nothing that could be tracked easily, but seems like a good improvement.

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

stevenzwu · 2023-12-18T23:01:37Z

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

+              wakeUp.wait(1000);
+            } catch (InterruptedException e) {
+              Thread.currentThread().interrupt();
+              throw new RuntimeException("Interrupted while blocked on reading.", e);


is it correct to throw RuntimeException? why don't we just propagate the InterruptedException per the contract?

* @throws IOException when encountered IO errors, such as deserialization failures. */ RecordsWithSplitIds<E> fetch() throws IOException;

also the error msg is inaccurate. it is not blocked on reading. it is paused due to watermark alignment

The issue is that the InterruptedException is not declared as throwable, so we can not throw it...

stevenzwu · 2023-12-18T23:46:57Z

...k/src/test/java/org/apache/iceberg/flink/source/TestIcebergSourceWithWatermarkExtractor.java

@@ -383,8 +381,10 @@ protected IcebergSource<RowData> source() {
        .project(TestFixtures.TS_SCHEMA)
        .splitSize(100L)
        .streaming(true)
-        .monitorInterval(Duration.ofMillis(2))
+        .monitorInterval(Duration.ofMillis(100))


2ms may cause high cpu usage. but why would it cause flakiness?

let's change it to 10 ms then. smaller value can discover new files sooner and make the test run faster. we already have a lot of integration tests using 10 ms and they are running fine.

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

stevenzwu · 2023-12-19T00:05:51Z

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

+      if (pausedSplits.contains(currentSplitId)) {
+        // Wait until the reader is unblocked. Wake every second to catch any missed signal.
+        // Return empty records if wakeUp is called, so pauseOrResumeSplits could be processed.
+        boolean first = true;


it seems that the logic can be significantly simpler if we use unlimited wait(). simply return an empty result upon wakeUp.wait()

I am generally very-very cautious about using loop-less blocking calls.
Here is an example:

WT (Watermark Thread) - Watermark arrives, and the decision is to block this reader

WT - wakeup is called

ST (SplitReader Thread) - Current fetch finished

ST - pauseOrResumeSplits is called

ST - New fetch called

ST - New fetch is checking if the split is paused, and find that we need to block, but do not start to wait on the lock yet.

WT - New watermark arrives, and the decision is to unblock the reader

WT - wakeup is called

ST - If the fetch arrives to the locking code now, then it will not receive the wakeup call

The main issue is that the wakeUp and the pauseOrResumeSplits is not atomic, we need another flag to store if the given wakeUp is handled, or not.

So i think we minimally need to have something like this:

if (pausedSplits.contains(currentSplitId)) { LOG.info("Reader {} paused reading split {}", indexOfSubtask, currentSplitId); synchronized (wakeUp) { if (wakeUp.get()) { wakeUp.set(false); return new RecordsBySplits(Collections.emptyMap(), Collections.emptySet()); } try { wakeUp.wait(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new RuntimeException("Interrupted while blocked on reading.", e); } } } }

I think compared to this the added security of the while loop is a good compromise.
WDYT?

I think we need the while loop, purely because there could be spurious wakeups and we may have to retry if wakeUp==false. I think the code snippet above you posted @pvary is a good compromise if you replace the if with a while.

@mxm According to the API contract, it is ok to return empty result in case of spurious wakeups. that is what the current PR does already.

if (wakeUp.get()) { wakeUp.set(false); return new RecordsBySplits(Collections.emptyMap(), Collections.emptySet()); }

@pvary I am fine with while-loop and using wait timeout. I understand it is safer for avoiding potential deadlock.

The main issue is that the wakeUp and the pauseOrResumeSplits is not atomic, we need another flag to store if the given wakeUp is handled, or not.

I see the problem now from the timeline example you said earlier.

The code snippet above, and the one proposed by the PR is only different because I wanted to add logging to the blocking path, but do not want to swamp the log with a new message every second. If you think that the logging is not important, the decision is that we accept that we log a line every second on INFO level until the SplitReader is unblocked, then we can simplify the code. Or we accept that every fetch() loop will create and new boolean variable which is not used in 90 percent of the cases.

On the hot path I prefer optimized code to simpler code, but the difference is probably not too noticeable anyway.

from the timeline example above, it indicates a condition where the reader thread can be unnecessary blocked for 1s, which is not ideal. trying to think if there is anyway we can avoid this suboptimal bhavior.

The timeline is only valid if we do not use an extra flag to store the wakeUp state and rely on a simple wait-notify pattern.

I used this timeline to describe why I introduced the wakeUp flag, and use synchronize around it in the PR.
I make sure that the wakeUp flag is only read/set inside a synchronized block, and we return from the fetch while holding the synchronization lock. So if there is a concurrent wakeUp method call, it will be blocked until we return, and we handle the next wakeup in the next fetch loop

I wanted to add logging to the blocking path, but do not want to swamp the log with a new message every second.

I think it is good to log this as TRACE only. we really need an INFO for the first instance, because we already have an INFO log in pauseOrResumeSplits method.

it seems that we might just need this while loop

while (pausedSplits.contains(currentSplitId)) { LOG.trace("Reader {} is still paused reading split {}", indexOfSubtask, currentSplitId); synchronized (wakeUp) { if (wakeUp.get()) { wakeUp.set(false); return new RecordsBySplits(Collections.emptyMap(), Collections.emptySet()); } try { wakeUp.wait(1000); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new IOException("Interrupted while paused due to watermark alignment.", e); }

...k/src/test/java/org/apache/iceberg/flink/source/TestIcebergSourceWithWatermarkExtractor.java

...flink/src/test/java/org/apache/iceberg/flink/source/reader/TestIcebergSourceSplitReader.java

mxm

Looks good to me.

mxm · 2023-12-19T16:31:53Z

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

+      if (pausedSplits.contains(currentSplitId)) {
+        // Wait until the reader is unblocked. Wake every second to catch any missed signal.
+        // Return empty records if wakeUp is called, so pauseOrResumeSplits could be processed.
+        boolean first = true;


I think we need the while loop, purely because there could be spurious wakeups and we may have to retry if wakeUp==false. I think the code snippet above you posted @pvary is a good compromise if you replace the if with a while.

stevenzwu · 2023-12-19T17:33:40Z

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

+              wakeUp.wait(1000);
+            } catch (InterruptedException e) {
+              Thread.currentThread().interrupt();
+              throw new RuntimeException("Interrupted while paused due to watermark alignment.", e);


@pvary sorry, I misspoken. I meant IOException instead of InterruptedException

Understable. The fetch javadoc says: or it can just throw an interrupted exception.
Set the type of the thrown exception to IOException

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

...flink/src/test/java/org/apache/iceberg/flink/source/reader/TestIcebergSourceSplitReader.java

mas-chen · 2023-12-19T19:55:08Z

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java

  @Override
-  public void wakeUp() {}
+  public void wakeUp() {


Should we wakeup if there are no pending splits? I'm trying to reason the previous behavior with wakeup being noop. I suppose wakeup was noop because the source only handles one split, so fetch and handleSplitChanges never occur together

I guess it is okay, since the spurious wakeup condition is caught in the fetch loop. And this way you can avoid synchronization on pausedSplits

...flink/src/test/java/org/apache/iceberg/flink/source/reader/TestIcebergSourceSplitReader.java

…ortedOperationException

pvary · 2023-12-21T07:33:48Z

Thanks @stevenzwu, @mas-chen, @nastra, @mxm and @rodmeneses for all the patience and the useful comments!

…ortedOperationException (apache#9308)

…pache#9403)

…ortedOperationException (apache#9308)

…pache#9403)

…ortedOperationException (apache#9308)

…pache#9403)

pvary requested review from stevenzwu and nastra December 15, 2023 15:23

github-actions bot added flink build labels Dec 15, 2023

pvary force-pushed the wm2 branch from 1d82823 to 4cd45e0 Compare December 15, 2023 16:05

rodmeneses reviewed Dec 15, 2023

View reviewed changes

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java Outdated Show resolved Hide resolved

rodmeneses reviewed Dec 15, 2023

View reviewed changes

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java Outdated Show resolved Hide resolved

rodmeneses reviewed Dec 15, 2023

View reviewed changes

...flink/src/test/java/org/apache/iceberg/flink/source/reader/TestIcebergSourceSplitReader.java Outdated Show resolved Hide resolved

rodmeneses approved these changes Dec 15, 2023

View reviewed changes

pvary mentioned this pull request Dec 16, 2023

Flink: Fix TestIcebergSourceWithWatermarkExtractor flakiness #9309

Merged

pvary force-pushed the wm2 branch from 4cd45e0 to 7534f5a Compare December 16, 2023 09:37

stevenzwu reviewed Dec 16, 2023

View reviewed changes

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java Outdated Show resolved Hide resolved

pvary force-pushed the wm2 branch from 3d92980 to 447f7b5 Compare December 17, 2023 11:37

pvary force-pushed the wm2 branch from 447f7b5 to bd8d436 Compare December 18, 2023 15:49

mas-chen reviewed Dec 18, 2023

View reviewed changes

pvary force-pushed the wm2 branch from 191f860 to 83044f9 Compare December 18, 2023 19:00

pvary commented Dec 18, 2023

View reviewed changes

stevenzwu reviewed Dec 19, 2023

View reviewed changes

pvary force-pushed the wm2 branch from a3e78ba to 52d9051 Compare December 19, 2023 12:35

mxm reviewed Dec 19, 2023

View reviewed changes

stevenzwu reviewed Dec 19, 2023

View reviewed changes

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java Outdated Show resolved Hide resolved

stevenzwu changed the title ~~Flink: Support watermark alignment of source splits~~ Flink: implement pause and resume in IcebergSourceSplitReader Dec 19, 2023

stevenzwu reviewed Dec 19, 2023

View reviewed changes

....18/flink/src/main/java/org/apache/iceberg/flink/source/reader/IcebergSourceSplitReader.java Outdated Show resolved Hide resolved

...flink/src/test/java/org/apache/iceberg/flink/source/reader/TestIcebergSourceSplitReader.java Outdated Show resolved Hide resolved

mas-chen reviewed Dec 19, 2023

View reviewed changes

nastra reviewed Dec 20, 2023

View reviewed changes

...flink/src/test/java/org/apache/iceberg/flink/source/reader/TestIcebergSourceSplitReader.java Outdated Show resolved Hide resolved

Flink: Empty implementation for pauseOrResumeSplits to prevent Unsupp…

63cfee4

…ortedOperationException

pvary force-pushed the wm2 branch from 800b335 to 63cfee4 Compare December 21, 2023 00:54

pvary changed the title ~~Flink: implement pause and resume in IcebergSourceSplitReader~~ Flink: Empty implementation for pauseOrResumeSplits to prevent UnsupportedOperationException Dec 21, 2023

stevenzwu approved these changes Dec 21, 2023

View reviewed changes

pvary merged commit 2eea697 into apache:main Dec 21, 2023
13 checks passed

pvary deleted the wm2 branch December 21, 2023 07:32

pvary pushed a commit to pvary/iceberg that referenced this pull request Jan 3, 2024

Flink: Backport apache#9308 to v1.17 and the relevant parts to v1.16

b46470f

pvary mentioned this pull request Jan 3, 2024

Flink: Backport #9308 to v1.17 and the relevant parts to v1.16 #9403

Merged

pvary pushed a commit to pvary/iceberg that referenced this pull request Jan 3, 2024

Flink: Backport apache#9308 to v1.17 and the relevant parts to v1.16

972a4b6

pvary added a commit that referenced this pull request Jan 3, 2024

Flink: Backport #9308 to v1.17 and the relevant parts to v1.16 (#9403)

8c7001e

lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024

Flink: Empty implementation for pauseOrResumeSplits to prevent Unsupp…

4368204

…ortedOperationException (apache#9308)

lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024

Flink: Backport apache#9308 to v1.17 and the relevant parts to v1.16 (a…

f55407c

…pache#9403)

geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024

Flink: Empty implementation for pauseOrResumeSplits to prevent Unsupp…

b7c5d1b

…ortedOperationException (apache#9308)

geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024

Flink: Backport apache#9308 to v1.17 and the relevant parts to v1.16 (a…

aab4d10

…pache#9403)

devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024

Flink: Empty implementation for pauseOrResumeSplits to prevent Unsupp…

67f06a1

…ortedOperationException (apache#9308)

devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024

Flink: Backport apache#9308 to v1.17 and the relevant parts to v1.16 (a…

42c7665

…pache#9403)

	assertThat(result.numberOfRecords()).isEqualTo(3);
	assertThat(result.numberOfRecords()).isGreaterThan(2);

Flink: Empty implementation for pauseOrResumeSplits to prevent UnsupportedOperationException #9308

Flink: Empty implementation for pauseOrResumeSplits to prevent UnsupportedOperationException #9308

Conversation

pvary commented Dec 15, 2023 • edited Loading

pvary commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodmeneses left a comment

Choose a reason for hiding this comment

stevenzwu commented Dec 15, 2023 • edited Loading

pvary commented Dec 15, 2023

stevenzwu left a comment

Choose a reason for hiding this comment

pvary commented Dec 17, 2023

pvary commented Dec 17, 2023

pvary commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mas-chen commented Dec 18, 2023

pvary commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvary Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzwu Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvary commented Dec 21, 2023

pvary commented Dec 15, 2023 •

edited

Loading

stevenzwu commented Dec 15, 2023 •

edited

Loading

pvary Dec 19, 2023 •

edited

Loading

stevenzwu Dec 19, 2023 •

edited

Loading