fix: remove race condition bug in refresh logic #1390

hessjcg · 2023-07-17T19:38:15Z

Update the logic in forceRefresh() to reduce the churn on the thread pool when the certificate refresh API calls are failing.

New forceRefresh() logic ensures that:

Only 1 refresh cycle may run at a time.
If a refresh cycle is in progress, then it will not be canceled until it succeeds.

Add new test cases to validate race conditions, deadlocks, and concurrency.

Add additional logging to help diagnose production problems with certificate refresh.

Related to #1314

Fixes #1209
Fixes #1159

…esh churn #1314

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java

enocom · 2023-07-17T20:03:54Z

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java

+  }
+
+  /** A constructor for use in tests that allows the caller to set the RateLimiter. */
+  CloudSqlInstance(


Let's just update the constructor to pass in the rate limiter. We don't need two constructors.

I prefer 2 constructors. I don't want to expose a public method to construct the default rate limiter just so that CoreSocketFactory can construct a CloudSqlInstance.

As a rule, I think we shouldn't be making test only APIs. And parameterizing the rate limiter is actually useful. Imagine if we wanted to expose rate limiting knobs to the user, or configure instances with different rate limits.

Fixed. I'm not convinced, but it's not worth arguing about.

enocom · 2023-07-17T20:05:24Z

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java

+        return;
+      }
+
+      forceRefreshRunning = true;
      nextInstanceData.cancel(false);
      if (nextInstanceData.isCancelled()) {


Shall we remove this if block and always schedule a next refresh since we also always call cancel? This after having observed that cancel doesn't always work as we might think.

Done. The if (isCanceled()) is now irrelevant since now we use the forceRefreshRunning field instead.

FYI: cancel works as expected: nextInstanceData.cancel(false) will cancel the task if it is queued, but not started. It will do nothing and return false if the task is running or done.

enocom · 2023-07-17T20:05:55Z

core/src/main/java/com/google/cloud/sql/core/CoreSocketFactory.java

@@ -120,9 +120,11 @@ public static synchronized CoreSocketFactory getInstance() {
  @VisibleForTesting
  // Returns a listenable, scheduled executor that exits upon shutdown.
  static ListeningScheduledExecutorService getDefaultExecutor() {
-    // TODO(kvg): Figure out correct way to determine number of threads
+    // Set the number of threads to 8. This should be enough threads that most users


Let's split this out into a separate PR since it's technically the fix of the bug.

Ok. See #1391

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

enocom · 2023-07-17T20:09:19Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+            newRateLimiter());
+
+    assertThat(supplier.counter.get()).isEqualTo(0);
+    Thread.sleep(500);


How can we avoid a sleep to ensure our test remains solid?

This is like an integration test of the ScheduledExecutorService. So this test is measuring how this JVM library class behaves when time passes. In this case, there is no way to mock the passing of time.

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

enocom · 2023-07-18T18:43:14Z

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java

+  }
+
+  /** A constructor for use in tests that allows the caller to set the RateLimiter. */
+  CloudSqlInstance(


As a rule, I think we shouldn't be making test only APIs. And parameterizing the rate limiter is actually useful. Imagine if we wanted to expose rate limiting knobs to the user, or configure instances with different rate limits.

enocom · 2023-07-18T18:43:56Z

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java

+      forceRefreshRunning = true;
+      nextInstanceData.cancel(false);
+      logger.fine(
+          "Force Refresh: the next refresh operation was cancelled."


Let's add the instance to the log message in leading brackets as we've done elsewhere. You might even make a convenience method for it that all methods (or even classes) can use.

enocom · 2023-07-18T18:59:14Z

core/src/main/java/com/google/cloud/sql/core/SqlAdminApiFetcher.java

+                  logger.info(
+                      String.format(
+                          "[%s %d] INSTANCE DATA DONE",
+                          instanceName, Thread.currentThread().getId()));


Let's leave the thread ID off -- unless it's worth keeping?

ditto with below

enocom · 2023-07-18T18:59:45Z

core/src/main/java/com/google/cloud/sql/core/SqlAdminApiFetcher.java

@@ -152,14 +151,22 @@ public InstanceData getInstanceData(
                            .orElse(x509Certificate.getNotAfter());
                  }

+                  logger.info(


These should all be logger.fine

enocom

I think we can test this directly. Let's talk tomorrow about how to do that.

enocom · 2023-07-18T20:32:13Z

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java

+  @GuardedBy("instanceDataGuard")
+  private boolean forceRefreshRunning;
+
+  static final RateLimiter defaultRateLimiter() {


Let's just pass this in and not expand the public API here. As an example, we pass in a RateLimiter in the AlloyDB Java Connector here: https://github.com/GoogleCloudPlatform/alloydb-java-connector/blob/main/alloydb-jdbc-connector/src/main/java/com/google/cloud/alloydb/Connector.java#L156.

enocom · 2023-07-18T20:53:05Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+
+    private final boolean flakey;
+
+    private AtomicInteger counter = new AtomicInteger();


let's make this final.

enocom · 2023-07-18T20:53:14Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    private final boolean flakey;
+
+    private AtomicInteger counter = new AtomicInteger();
+    private InstanceData response =


Ditto here.

enocom · 2023-07-18T20:53:30Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+                null),
+            new SslData(null, null, null),
+            Date.from(Instant.now().plus(1, ChronoUnit.HOURS)));
+    private final ListeningScheduledExecutorService executor;


This can be a local variable.

enocom · 2023-07-18T20:57:31Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+
+  private static class TestDataSupplier implements InstanceDataSupplier {
+
+    private final boolean flakey;


core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

enocom

I see the new tests here add ~1 minute to our build. I think there's a way we can test this without the time increase. Let's talk about how when you're ready.

enocom · 2023-07-19T19:43:54Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    MockAdminApi mockAdminApi = new MockAdminApi();
+    ListenableFuture<KeyPair> keyPairFuture =
+        Futures.immediateFuture(mockAdminApi.getClientKeyPair());
+    ListeningScheduledExecutorService executor = newTestExecutor();


Since newTestExecutor is a call through to the actual executor, shall we just inline this method?

Fixed. Method inlined.

enocom · 2023-07-19T19:47:49Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+                int c = counter.incrementAndGet();
+                Thread.sleep(100);
+
+                if (flaky && c % 2 == 0 && c > 10) {


I had added the greater than 10 in to allow Hikari to establish two pools of 5 connections each successfully before simulating flaky behavior. Does this need to be here for test?

Nope. I removed it.

enocom · 2023-07-19T19:49:09Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    // Attempt to retrieve data, ensure we wait for success
+    ListenableFuture<List<Object>> allData =
+        Futures.allAsList(
+            executor.submit(instance::getSslData),


What is the significant of three here?

Three would make multiple, simultaneous calls to getSslData().

But why three? Why not four, or two?

It just needs to be more than one. And three is a magic number.

Let's use two in that case, and add a comment. I'm asking for my personal interest, but also for future readers who will wonder the same.

enocom · 2023-07-19T19:49:40Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+              executor.submit(instance::forceRefresh));
+      try {
+        all.get();
+      } catch (Exception e) {


Why are we silently squashing the exception here? Should we add it to the test signature instead?

I've updated this method significantly and added comments. Please take another look.

enocom · 2023-07-19T19:51:08Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+        allData2.get();
+      } catch (Exception e) {
+      }
+      assertThat(supplier.counter.get()).isEqualTo(2 + i);


Why is the supplier equal to 2 plus i? Shouldn't the supplier be called once for every completed force refresh?

Also, this should be more clear now.

enocom · 2023-07-19T19:51:41Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+  }
+
+  @Test
+  public void testCloudSqlInstanceCorrectlyRefreshesInstanceData() throws Exception {


How is this different from a happy path test we have elsewhere? Do we need it here?

This is the happy path without any concurrency. Before I changed the mainline code, this test passed and the other tests with more concurrency failed. It was an important test to demonstrate that the bad behavior was caused by concurrency bugs.

If this wasn't used to drive out the bug, I think we should check that we have similar coverage elsewhere (I believe we do) and omit this one here.

We do not have exactly this same coverage elsewhere. This was used to drive out the bug. I would like to leave this here.

I think this test is just the same as this test. In my view, the assertions don't communicate what different behavior we have.

enocom · 2023-07-19T19:52:22Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+            newRateLimiter());
+
+    Thread.sleep(500);
+    Assert.assertThrows(RuntimeException.class, () -> instance.getSslData());


Same question here? How does this test differ from other tests we already have?

The key difference is that this one throws an exception from within a scheduled executor task. This used to exacerbate the deadlock problem.

Is this behavior covered by testCloudSqlInstanceRefreshesConsistentlyWithoutRaceConditions? It looks like a duplicate test to my eyes.

This test has a single CloudSqlInstance and ensures that it successfully refreshes the correct number of times. Thus proving that the balking behavior of forceRefresh() works right.

testCloudSqlInstanceRefreshesConsistentlyWithoutRaceConditions tries to cause a race condition and deadlock with multiple CloudSqlInstances and many more threads. It is fundimentally a different test.

While developing, if there was a bug in my code, one of these tests would fail and the other would pass. So I think they are both useful.

Right now this test proves that the CloudSqlInstance calls the underlying InstanceDataSupplier, which is a variation of this test. If you'd like to test that force refresh functions correctly, then I'd expect a call that fails, a force refresh, and then a call that succeeeds.

enocom · 2023-07-19T20:00:10Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    }
+
+    // Get SSL Data for each instance, forcing the first refresh to complete.
+    instances.forEach(i -> i.getSslData());


This can be simplified to a lambda reference.

hessjcg

I updated the CloudSqlInstanceConcurrencyTest based on comments.

hessjcg · 2023-07-19T23:20:11Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+                int c = counter.incrementAndGet();
+                Thread.sleep(100);
+
+                if (flaky && c % 2 == 0 && c > 10) {


Nope. I removed it.

hessjcg · 2023-07-19T23:20:44Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    MockAdminApi mockAdminApi = new MockAdminApi();
+    ListenableFuture<KeyPair> keyPairFuture =
+        Futures.immediateFuture(mockAdminApi.getClientKeyPair());
+    ListeningScheduledExecutorService executor = newTestExecutor();


Fixed. Method inlined.

hessjcg · 2023-07-19T23:21:29Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    // Attempt to retrieve data, ensure we wait for success
+    ListenableFuture<List<Object>> allData =
+        Futures.allAsList(
+            executor.submit(instance::getSslData),


Three would make multiple, simultaneous calls to getSslData().

hessjcg · 2023-07-19T23:55:13Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+              executor.submit(instance::forceRefresh));
+      try {
+        all.get();
+      } catch (Exception e) {


I've updated this method significantly and added comments. Please take another look.

hessjcg · 2023-07-19T23:55:27Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+        allData2.get();
+      } catch (Exception e) {
+      }
+      assertThat(supplier.counter.get()).isEqualTo(2 + i);


Also, this should be more clear now.

hessjcg · 2023-07-19T23:56:18Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+  }
+
+  @Test
+  public void testCloudSqlInstanceCorrectlyRefreshesInstanceData() throws Exception {


This is the happy path without any concurrency. Before I changed the mainline code, this test passed and the other tests with more concurrency failed. It was an important test to demonstrate that the bad behavior was caused by concurrency bugs.

hessjcg · 2023-07-20T00:00:18Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+            newRateLimiter());
+
+    Thread.sleep(500);
+    Assert.assertThrows(RuntimeException.class, () -> instance.getSslData());


The key difference is that this one throws an exception from within a scheduled executor task. This used to exacerbate the deadlock problem.

hessjcg · 2023-07-20T00:00:43Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    }
+
+    // Get SSL Data for each instance, forcing the first refresh to complete.
+    instances.forEach(i -> i.getSslData());


enocom · 2023-07-20T16:33:07Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    // Attempt to retrieve data, ensure we wait for success
+    ListenableFuture<List<Object>> allData =
+        Futures.allAsList(
+            executor.submit(instance::getSslData),


But why three? Why not four, or two?

enocom · 2023-07-20T17:20:54Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    }
+  }
+
+  @Test(timeout = 45000)


Do we still need this?

Yes. If there is a deadlock, then the test will only fail with a timeout. So we should leave this in just incase we cause a deadlock again in the future.

Do we really want to wait 45 seconds before failing? What's the default value?

enocom · 2023-07-20T17:21:40Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+  }
+
+  @Test
+  public void testCloudSqlInstanceCorrectlyRefreshesInstanceData() throws Exception {


If this wasn't used to drive out the bug, I think we should check that we have similar coverage elsewhere (I believe we do) and omit this one here.

enocom · 2023-07-20T17:22:24Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+            newRateLimiter());
+
+    Thread.sleep(500);
+    Assert.assertThrows(RuntimeException.class, () -> instance.getSslData());


Is this behavior covered by testCloudSqlInstanceRefreshesConsistentlyWithoutRaceConditions? It looks like a duplicate test to my eyes.

enocom · 2023-07-20T17:22:34Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    // exception after the first refresh attempt fails.
+  }
+
+  @Test(timeout = 45000) // 45 seconds timeout in case of deadlock


Same thing here. Do we need this?

Yes. If this test deadlocks, then the test will fail with a timeout. Otherwise it will hang indefinitely.

Same here -- can we bump this number down? Can we rely on the default value? I assume the test framework does a default timeout (I think it's 30s).

enocom · 2023-07-20T17:39:32Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    }
+  }
+
+  @Test(timeout = 45000)


Do we really want to wait 45 seconds before failing? What's the default value?

enocom · 2023-07-20T17:39:59Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    // Attempt to retrieve data, ensure we wait for success
+    ListenableFuture<List<Object>> allData =
+        Futures.allAsList(
+            executor.submit(instance::getSslData),


Let's use two in that case, and add a comment. I'm asking for my personal interest, but also for future readers who will wonder the same.

enocom · 2023-07-20T17:40:43Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+          // We expect some of these to throw an exception indicating that the refresh cycle
+          // got a failed attempt. When they throw an exception,
+          // sleep and try again. This shows that the refresh cycle is working.
+          Thread.sleep(100);


Shall we add a log statement here too?

enocom · 2023-07-20T17:42:48Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+  }
+
+  @Test
+  public void testCloudSqlInstanceCorrectlyRefreshesInstanceData() throws Exception {


I think this test is just the same as this test. In my view, the assertions don't communicate what different behavior we have.

enocom · 2023-07-20T17:43:44Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+            keyPairFuture,
+            newRateLimiter());
+
+    Thread.sleep(500);


Why the sleep here?

enocom · 2023-07-20T17:46:10Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+            newRateLimiter());
+
+    Thread.sleep(500);
+    Assert.assertThrows(RuntimeException.class, () -> instance.getSslData());


Right now this test proves that the CloudSqlInstance calls the underlying InstanceDataSupplier, which is a variation of this test. If you'd like to test that force refresh functions correctly, then I'd expect a call that fails, a force refresh, and then a call that succeeeds.

enocom · 2023-07-20T17:46:40Z

core/src/test/java/com/google/cloud/sql/core/CloudSqlInstanceConcurrencyTest.java

+    // exception after the first refresh attempt fails.
+  }
+
+  @Test(timeout = 45000) // 45 seconds timeout in case of deadlock


Same here -- can we bump this number down? Can we rely on the default value? I assume the test framework does a default timeout (I think it's 30s).

fix: Increase threadpool count to avoid deadlock and reduce forceRefr…

c1c7763

…esh churn #1314

hessjcg requested a review from a team as a code owner July 17, 2023 19:38

blunderbuss-gcf bot assigned enocom Jul 17, 2023

fix: Increase threadpool count to avoid deadlock and reduce forceRefr…

17953f8

…esh churn #1314

hessjcg commented Jul 17, 2023

View reviewed changes

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java Show resolved Hide resolved

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java Show resolved Hide resolved

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java Show resolved Hide resolved

enocom requested changes Jul 17, 2023

View reviewed changes

hessjcg added 2 commits July 18, 2023 09:35

chore: add logging from enocom-debug branch

c9baaad

chore: code review comments.

a9557d6

hessjcg changed the title ~~fix: Increase threadpool count to avoid deadlock~~ fix: Fix race condition bug in refresh logic and add logging Jul 18, 2023

chore: code review comments.

90edd08

hessjcg requested a review from enocom July 18, 2023 16:46

enocom changed the title ~~fix: Fix race condition bug in refresh logic and add logging~~ fix: remove race condition bug in refresh logic Jul 18, 2023

enocom requested changes Jul 18, 2023

View reviewed changes

hessjcg force-pushed the gh-1314-increase-threadpool-count branch from 5f1a399 to 2c3d6cc Compare July 18, 2023 19:00

chore: better code comments.

2400ac0

hessjcg requested a review from enocom July 18, 2023 19:13

hessjcg added 2 commits July 18, 2023 13:44

chore: fix test timeouts.

ccb978d

chore: code formatting

875cde6

hessjcg force-pushed the gh-1314-increase-threadpool-count branch from a5958e4 to 875cde6 Compare July 18, 2023 20:01

fix: add copyright header.

5c18810

enocom mentioned this pull request Jul 18, 2023

bad_certificate errors intermittently preventing connection to cloud sql #1314

Closed

enocom requested changes Jul 18, 2023

View reviewed changes

fix: code review comments

80001b2

hessjcg requested a review from enocom July 19, 2023 16:55

enocom reviewed Jul 19, 2023

View reviewed changes

fix: reduce testing time by doing fewer iterations.

44dac45

hessjcg requested a review from enocom July 19, 2023 18:36

hessjcg enabled auto-merge (squash) July 19, 2023 18:44

enocom reviewed Jul 19, 2023

View reviewed changes

hessjcg mentioned this pull request Jul 19, 2023

fix: Use guava rate limiter instead of dev.failsafe #1393

Merged

chore: code review comments.

a677341

hessjcg commented Jul 20, 2023

View reviewed changes

hessjcg requested a review from enocom July 20, 2023 15:47

enocom reviewed Jul 20, 2023

View reviewed changes

hessjcg added 2 commits July 20, 2023 14:42

fix: better testcases.

e84b28b

fix: add copyright header.

b514c26

enocom approved these changes Jul 20, 2023

View reviewed changes

hessjcg merged commit c0a5d58 into main Jul 20, 2023
17 checks passed

hessjcg deleted the gh-1314-increase-threadpool-count branch July 20, 2023 21:11

infra-db-release-bot mentioned this pull request Jul 20, 2023

chore(main): release 1.13.1 #1392

Merged

This was referenced Jul 20, 2023

sql.core.R2dbcPostgresIntegrationTests: pooledConnectionTest failed #1342

Closed

sql.postgres.JdbcPostgresIamAuthIntegrationTests: pooledConnectionTest failed #1335

Closed

enocom mentioned this pull request Aug 30, 2023

Port recent force refresh and threading improvements from Cloud SQL Java Connector GoogleCloudPlatform/alloydb-java-connector#87

Closed


		private final boolean flakey;

		private AtomicInteger counter = new AtomicInteger();


		private static class TestDataSupplier implements InstanceDataSupplier {

		private final boolean flakey;

fix: remove race condition bug in refresh logic #1390

fix: remove race condition bug in refresh logic #1390

Conversation

hessjcg commented Jul 17, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enocom 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

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

enocom 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

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

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

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

hessjcg 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

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

Choose a reason for hiding this comment

hessjcg commented Jul 17, 2023 •

edited

Loading