CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) by AshenScribe · Pull Request #4953 · apache/cassandra

AshenScribe · 2026-07-22T03:37:16Z

AshenScribe

StorageService.drain() was synchronized on the same monitor as drain(boolean). Pre-CEP-59, drain() called drain(false) directly on the same thread, so reentrancy made this safe. Now drain() hands a callback to gracefulDisconnect(...), which invokes it asynchronously (Netty close-listener or scheduler thread) — so drain(false) can run on a different thread than the one blocked in drain()'s await(). Since that thread still holds the monitor while waiting, drain(false) can never acquire it → deadlock whenever a client is still connected at drain time.

Fix: drop synchronized from drain(), use a dedicated ReentrantLock scoped only to drain() (preserves "one drain at a time"), leave drain(boolean)'s own synchronized untouched.

Copilot

Pull request overview

Implements CEP-59 graceful native-protocol disconnection for controlled Cassandra shutdowns.

Changes:

Adds capability advertisement, event registration, and connection draining.
Integrates draining with shutdown operations and metrics.
Adds configuration, documentation, and tests.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
`test/unit/org/apache/cassandra/service/StorageServiceTest.java`	Tests graceful-disconnect settings.
`test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java`	Tests configuration defaults and updates.
`test/unit/org/apache/cassandra/concurrent/DebuggableScheduledThreadPoolExecutorTest.java`	Propagates the new drain exception.
`test/distributed/org/apache/cassandra/distributed/test/GracefulDisconnectTest.java`	Adds distributed feature tests.
`src/java/org/apache/cassandra/transport/SimpleClient.java`	Handles graceful-disconnect events.
`src/java/org/apache/cassandra/transport/Server.java`	Tracks subscribers and stops accepting connections.
`src/java/org/apache/cassandra/transport/messages/StartupMessage.java`	Defines the capability key.
`src/java/org/apache/cassandra/transport/messages/OptionsMessage.java`	Advertises feature support.
`src/java/org/apache/cassandra/transport/InitialConnectionHandler.java`	Advertises support before negotiation.
`src/java/org/apache/cassandra/transport/Event.java`	Defines the protocol event.
`src/java/org/apache/cassandra/tools/nodetool/Drain.java`	Handles drain timeout errors.
`src/java/org/apache/cassandra/tools/NodeProbe.java`	Propagates drain timeouts.
`src/java/org/apache/cassandra/service/StorageServiceMBean.java`	Exposes settings and timeout contract.
`src/java/org/apache/cassandra/service/StorageService.java`	Orchestrates graceful shutdown.
`src/java/org/apache/cassandra/service/NativeTransportService.java`	Exposes subscribed channels.
`src/java/org/apache/cassandra/metrics/ClientMetrics.java`	Adds draining metrics.
`src/java/org/apache/cassandra/config/DatabaseDescriptor.java`	Exposes configuration accessors.
`src/java/org/apache/cassandra/config/Config.java`	Defines feature configuration.
`doc/modules/cassandra/pages/managing/operating/graceful_disconnect.adoc`	Documents operation and compatibility.
`conf/cassandra.yaml`	Documents stable configuration defaults.
`conf/cassandra_latest.yaml`	Enables the feature in latest configuration.

Suppressed comments (1)

src/java/org/apache/cassandra/transport/SimpleClient.java:85

Removing this suppression makes Netty Promise fail the IllegalImport check in .build/checkstyle.xml:104. Restore the established import-level suppression.

import io.netty.util.concurrent.Promise;

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

 import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.CountDownLatch;


 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.ConcurrentLinkedQueue;
-import java.util.concurrent.SynchronousQueue; // checkstyle: permit this import
+import java.util.concurrent.SynchronousQueue;


SiyaoIsHiding · 2026-08-14T01:04:24Z

                    supportedOptions.put(StartupMessage.CQL_VERSION, cqlVersions);
                    supportedOptions.put(StartupMessage.COMPRESSION, compressions);
                    supportedOptions.put(StartupMessage.PROTOCOL_VERSIONS, ProtocolVersion.supportedVersions());
+                    supportedOptions.put(StartupMessage.GRACEFUL_DISCONNECT, List.of(String.valueOf(DatabaseDescriptor.getGracefulDisconnectEnabled())));


+            if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS))
+                throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect");


+                if (connectedChannels.decrementAndGet() == 0)
+                {
+                    timeoutTask.cancel(false);
+                    runOnceAction.run();


SiyaoIsHiding · 2026-09-01T23:05:36Z

+    public boolean graceful_disconnect_enabled = false;
+
+    public volatile DurationSpec.LongMillisecondsBound graceful_disconnect_grace_period = new DurationSpec.LongMillisecondsBound(5000);


Isn't this the default configuration itself?

+    @Test
+    public void testGracefulDisconnectEnabled()
+    {
+        Assertions.assertThat(StorageService.instance.getGracefulDisconnectEnabled()).isFalse();


+        AtomicBoolean actionStarted = new AtomicBoolean(false);
+        AtomicInteger connectedChannels = new AtomicInteger(channelGroup.size());


+        if (bindChannel != null && bindChannel.isOpen())
+        {
+            logger.info("Stopping native transport acceptor on {}", bindChannel.localAddress());
+            // syncUninterruptibly ensures we wait for the port to actually close
+            bindChannel.close().syncUninterruptibly();


+
+== Solution
+
+Introduce an in-band signal — `GRACEFUL_DISCONNECT` — so that the server can notify clients (that have subscribed) connection before closing it. This gives drivers time to:


SiyaoIsHiding

Preliminary review, yet to dig into the core mechanism.
The most convincing test is to put together a client-server scenario, where it throws an error when graceful disconnect is disabled, and does not throw an error when graceful disconnect is enabled. This is the whole purpose of CEP-59.
But the existing tests are only about configs, whether server sends event or start draining, etc. We need the test to prove the error is gone.
If the existing integration test harness is not enough to put together such scenario, then a manual testing is needed.

SiyaoIsHiding · 2026-08-14T00:45:18Z

            connection.setCompressor(Compressor.LZ4Compressor.instance);
        }
+        if (version.isGreaterOrEqualTo(ProtocolVersion.V5))
+            options.put(StartupMessage.GRACEFUL_DISCONNECT, "GRACEFUL_DISCONNECT");


graceful disconnect is opted in by REGISTER, not in startup message

SiyaoIsHiding · 2026-08-14T01:04:24Z

                    supportedOptions.put(StartupMessage.CQL_VERSION, cqlVersions);
                    supportedOptions.put(StartupMessage.COMPRESSION, compressions);
                    supportedOptions.put(StartupMessage.PROTOCOL_VERSIONS, ProtocolVersion.supportedVersions());
+                    supportedOptions.put(StartupMessage.GRACEFUL_DISCONNECT, List.of(String.valueOf(DatabaseDescriptor.getGracefulDisconnectEnabled())));


SiyaoIsHiding · 2026-08-14T01:26:30Z

+                }
+            });
+            if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS))
+                throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect");


Why does drain throws a TimeoutException when graceful period passes? it should at most log a warning and proceed IMO

SiyaoIsHiding · 2026-08-14T01:37:26Z

    @VisibleForTesting
    Gauge<Integer> connectedNativeClients;

+    public AtomicInteger connectionsDraining;


We need tests for metrics too

AshenScribe · 2026-08-15T05:34:39Z

@SiyaoIsHiding I need a java driver supporting cep 59. I am unable to get one, so further testing can't be done by me.

SiyaoIsHiding · 2026-08-19T03:00:11Z

@AshenScribe I sent you a working java driver and a working native protocol on Feb 25, I also hopped on an half hour meeting with you around the same time when I showed you how to do necessary end-to-end testing.

AshenScribe · 2026-08-19T03:08:53Z

Hi @SiyaoIsHiding yes I remember that meeting. I'll do so.

As of drivers, since shanzita took over the java driver, I was waiting for her branch of native protocol, as that will be the official code to be merged now, which I received just yesterday.

SiyaoIsHiding · 2026-08-19T04:47:37Z

She didn't have a full server side implementation to test against until last month, either. You two needs each other to do end-to-end testing, so you both will need to test against WIP code, and you should expect changes through out the PR review process.

Shanzita · 2026-08-19T18:34:55Z

 import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
+import org.apache.cassandra.utils.concurrent.CountDownLatch


I think you missed a semicolon at the end of this import.

yup, I also skipped the import style check skip patching it now.

Also wait some moment before reviewing the StorageService class, I am going to change the approach there.

AshenScribe · 2026-08-20T13:06:13Z

added new logic for draining.
Todo

Write comprehensive test cases for GracefulDisconnectLifecycle, it contains basic ones only
Write comprehensive test cases for GracefulDisconnect feature, it contains basic ones only
Write comprehensive test cases for new metrics
Do manual testing using new java driver and native-protocol and this PR. Current test include draining with graceful_disconnect_enabled set to true and false and used cqlsh to test legacy behaviour.

SiyaoIsHiding · 2026-08-21T07:38:14Z

Here are a list of different ways for a node to disconnect and whether graceful disconnect should kick in.

Way to Disconnect	Graceful disconnect?
nodetool disablebinary	yes
nodetool disablebinary -f ("Use -f to interrupt client requests that have already started")	no
nodetool drain	yes
nodetool stopdaemon	yes
nodetool decommission	yes
nodetool decommission -f	yes
JVM shutdown hook/ SIGTERM/ systemctl stop	yes
ccm node1 stop (SIGTERM)	yes
SIGKILL	no

If everyone agree with the above behavior, this table should go into the CEP and the operating manual.

Assuming we want the above behavior, I'd suggest we add graceful disconnect to NativeTransportService.stop(bool force) when force == false, because all ways above that should gracefully disconnect calls NativeTransportService.stop(false).

Way to Disconnect	Callstack to graceful disconnect
nodetool disablebinary	StorageService.stopNativeTransport(bool force) -> CassandraDaemon.stopNativeTransport(bool force) -> NativeTransportService.stop(bool force)
nodetool disablebinary -f	Same as above, and it calls NativeTransportService.stop(true), so it should skip the graceful disconnect
nodetool drain	StorageService.drain(bool) -> StorageService.shutdownClientServers() -> StorageService.stopNativeTransport(false)
nodetool stopdaemon	CassandraDaemon.stop() -> CassandraDaemon.destroyClientTransports() -> NativeTransportService.stop()
nodetool decommission	StorageService.decommission() -> SingleNodeSequences.decommission(true, force) -> StorageService.shutdownNetowrking() -> StorageService.shutdownClientServers()
nodetool decommission -f	Exactly same above
JVM shutdown hook/ SIGTERM/ systemctl stop	Runtime.getRuntime().addShutdownHook(drainOnShutdown); -> StorageService.drain(true) -> shutdownClientServers()
SIGKILL	not handled

AshenScribe · 2026-08-25T07:53:56Z

@SiyaoIsHiding I inspected the files to find out call stack. I made this call stack graph.

Let me know if this aligns with your understand of drain flow.

So here only drain change is in Transport layer.

AshenScribe · 2026-08-25T09:08:39Z

I have added necessary test cases in the codebase.

Now starting manual testing. using this branch for server side, for Java Driver and native-protocol and stable java driver and stable native-protocol as un supported client.

To avoid redundant test cases across topologies (single vs multi node) and scale(flag true/false and driver supporting/non supportign), the 16 theoretical permutations cut down to 4 scenarios (and a mixed-driver scenario).

Needed Tests
Supporting Driver + Feature Enabled: Validates event registration, clean in-flight request draining, and clean socket closure (ForcedDisconnects = 0).

Supporting Driver + Feature Disabled: Validates that when disabled, capabilities are not advertised and the feature safely stays dormant.

Legacy Driver + Feature Enabled (Backward Compatibility): Validates that non-supporting drivers receive no events and fall back safely to standard TCP teardown without errors.

Uncooperative Supporting Driver (Timeout/Force-Close): Validates that if a driver ignores the event, the server honors graceful_disconnect_grace_period, forcefully severs the socket, and increments ForcedDisconnects.

Mixed-Fleet (Supporting + Legacy Clients Simultaneously Connected): Validates that supporting clients drain gracefully while legacy clients coexist on the same node without disruption and close on final server teardown.

Redundant test cases
Multi-Node vs. Single-Node Topologies: Graceful disconnect is a node-local Netty transport feature; cluster topology size does not alter connection teardown behavior.

Multi-Client Scaling Variations: Scaling from 1 client to 100 clients uses the exact same Netty ChannelGroup and atomic counters, making multi-client variations redundant once single-client and concurrent-client behaviors are verified in the codebase itself.

SiyaoIsHiding · 2026-08-25T18:56:16Z

The chart looks generally correct to me!

Re: GracefulDisconnectLifecycle, I think there might be a simplier and a cleaner way than a finite state machine. Now the only logic starts in either NativeTransportService.stop(bool) or Server.stop() but not in StorageService, the states should be simplier to manage. I'm still exploring cleaner ways to do that, you can try that out, too.

Re: the four testing scenarios you listed, you should check client side logs and see whether they are expected, too, apart from the server side logs.

Re: single node v.s. multi node, I personally prefer testing against multi node cluster because the primary purpose of graceful disconnect is that during rolling restarts, i.e. one node out of several is temporary down, the client application should be able to send the queries to other nodes so the client app can continue running, i.e. every request can get a response in time. If you use only single node and it's temporarily down, even if you have graceful disconnect, the client app will need to stop anyway because there just isn't any node that can give responses, most likely throwing AllNodesFailedException.
So I'd suggest you test against multi node cluster too.

AshenScribe · 2026-08-25T19:16:05Z

If I understand correctly:

grace_disconnect is a connection-local event, so the server behaves identically whether it's a single node or a multi-node cluster.

once the server emits GRACEFUL_DISCONNECT, routing subsequent requests to other healthy nodes in topology is entirely the driver's responsibility.

The FSM introduced traverses on two possible paths WAITING_FOR_CLIENTS → COMPLETE when all clients cooperate or WAITING_FOR_CLIENTS → FORCE_CLOSING not all cooperate, so not that difficult of a path. I'll keep looking for better logic though.

SiyaoIsHiding · 2026-08-25T19:28:18Z

Both your goal and Shanzita's goal is to eliminate errors on the driver's side, cuz that's this CEP's goal. So, you should check client side logs too cuz even if the server log shows what you expected, are you 100% sure the client side errors are gone? If you see the client side error persisting, are you 100% sure it's not the server side that's not behaving as you expected?

Re: multi-node, when you check the client side logs of your testing scenarios you will see multi-node clusters are easiest to check whether the client side errors are gone, cuz it's the difference of an application exiting or not. It's ultimately your choice tho.

SiyaoIsHiding · 2026-09-01T22:57:15Z

Please refer to https://github.com/SiyaoIsHiding/cassandra/blob/feature/cep-59/metrics/src/java/org/apache/cassandra/transport/Server.java
On how to implement the draining logic in a much simpler way, without the life cycle class.

AshenScribe · 2026-09-02T05:25:17Z

I appreciate the newCloseFuture().awaitUninterruptibly(...) approach, I'll adapot it in my implementation.

absurdfarce · 2026-09-02T21:10:31Z

        SCHEMA_CHANGE(ProtocolVersion.V3),
-        TRACE_COMPLETE(ProtocolVersion.V4);
+        TRACE_COMPLETE(ProtocolVersion.V4),
+        GRACEFUL_DISCONNECT(ProtocolVersion.V5);


There's no logical connection between the protocol version bound here and support for graceful disconnect; in theory most (all?) protocol versions could support it without issue. But changing this to even v4 would require some additional testing and it's not abundantly clear there's a huge population in the intersection of v4 + graceful disconnect.

It's also something we can change if we get pushback. A very reasonable compromise is to roll out the initial version with explicit support for v5 only and expand that to v4 if users register complaints.

I'm stealing most of this from some observations made by @SiyaoIsHiding in direct conversation recently. I believe she's going to be posting a follow-up to this effect in the not-too-distant future... I'm just noting it here since she asked me to take a look at this code. :)

SiyaoIsHiding

I'd propose we support Graceful Disconnect from protocol v4 and up, instead of v5, so quite some changes in this PR need to be updated, unless people disagree.
We need to update the file native_protocol_v4.spec and native_protocol_v5.spec too.
We also need to remove the redundant stopNativeTransport() call in CassandraDaemon.destroyClientTransports() as https://issues.apache.org/jira/projects/CASSANDRA/issues/CASSANDRA-21641.
And please avoid force-pushing when the PR is still under active review, so it's easier for reviewers to see what have changes since last review.

SiyaoIsHiding · 2026-09-01T23:04:09Z

        return server;
    }

+    public ChannelGroup getChannelsSubscribedToGracefulDisconnect()


Add

// for testing purpose

Cuz it's not used anywhere in src.

Ig we must use @VisibleForTesting annotation

SiyaoIsHiding · 2026-09-01T23:04:50Z

            probe.drain();
-        } catch (IOException | InterruptedException | ExecutionException e)
+        }
+        catch (IOException | InterruptedException | ExecutionException e)


What's the purpose of this change

SiyaoIsHiding · 2026-09-01T23:05:36Z

+    public boolean graceful_disconnect_enabled = false;
+
+    public volatile DurationSpec.LongMillisecondsBound graceful_disconnect_grace_period = new DurationSpec.LongMillisecondsBound(5000);


SiyaoIsHiding · 2026-09-01T23:06:12Z

@@ -0,0 +1,153 @@
+/*


Please refer to https://github.com/SiyaoIsHiding/cassandra/blob/feature/cep-59/metrics/src/java/org/apache/cassandra/transport/Server.java
On simplier draining logic.

SiyaoIsHiding · 2026-09-01T23:11:12Z

 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Queue;


Please revert the formatting changes in this file and do not move things around, e.g. that Builder, so it's easier for PR reviewers to review what's actually relevant to CEP-59

SiyaoIsHiding · 2026-09-02T23:36:59Z

@@ -239,4 +239,20 @@ public void testConnectedClientsAndAuthMetrics() throws SSLException
        assertEquals(0, clientMetrics.encryptedConnectedNativeClients.getValue().intValue());
        assertEquals(0, passwordConnections.getValue().intValue());
    }
+
+    @Test
+    public void testGracefulDisconnectMetrics()


This metrics test doesn't cover anything in the core mechanism. Please refer to other client metrics tests.

AshenScribe · 2026-09-03T06:16:53Z

the most recent force push was for in complete responses to changes requested..

AshenScribe · 2026-09-03T07:27:34Z

+            if (type == Event.Type.GRACEFUL_DISCONNECT && !DatabaseDescriptor.getGracefulDisconnectEnabled())
+                return;


Should we or should we not consider case of a buggy client sending GRACEFUL_DISCONNECT in register regardless of false value of that flag in supported options.

AshenScribe commented Jul 22, 2026

View reviewed changes

SiyaoIsHiding requested a balanced review from Copilot August 14, 2026 00:07

Copilot started reviewing on behalf of SiyaoIsHiding August 14, 2026 00:07 View session

Copilot AI reviewed Aug 14, 2026

View reviewed changes

SiyaoIsHiding reviewed Aug 14, 2026

View reviewed changes

SiyaoIsHiding changed the title ~~cep 59~~ CASSANDRA-21191: [CEP-59] Implementation of In-Band Connection Draining (Graceful Disconnect) Aug 14, 2026

Shanzita reviewed Aug 19, 2026

View reviewed changes

AshenScribe force-pushed the feature/cep-59/metrics branch from bdeaf90 to c0641da Compare August 20, 2026 12:59

AshenScribe force-pushed the feature/cep-59/metrics branch from c0641da to d9a366e Compare August 25, 2026 07:44

AshenScribe force-pushed the feature/cep-59/metrics branch from d9a366e to 406f620 Compare August 25, 2026 08:54

Shanzita mentioned this pull request Aug 26, 2026

CASSJAVA-124: Add GRACEFUL_DISCONNECT support (CEP-59) apache/cassandra-java-driver#2091

Open

cep 59

35e6cf1

AshenScribe force-pushed the feature/cep-59/metrics branch from 406f620 to 35e6cf1 Compare August 30, 2026 18:40

absurdfarce reviewed Sep 2, 2026

View reviewed changes

SiyaoIsHiding requested changes Sep 2, 2026

View reviewed changes

Rishabh Saraswat added 2 commits September 3, 2026 10:57

reformated SimpleClient.java

635137a

removed Lifecycle way of Draining

3ee100e

AshenScribe force-pushed the feature/cep-59/metrics branch from 74fc95e to 3ee100e Compare September 3, 2026 06:16

added client metrics tests

0694e84

AshenScribe commented Sep 3, 2026

View reviewed changes

removed ambuguity from functions names and a race condition

a325bad

AshenScribe force-pushed the feature/cep-59/metrics branch from 47acc4d to a325bad Compare September 3, 2026 07:31

		if (!drainComplete.await(DatabaseDescriptor.getGracefulDisconnectGracePeriod(), MILLISECONDS))
		throw new TimeoutException("Timed out waiting for drain to complete after graceful disconnect");

		public boolean graceful_disconnect_enabled = false;

		public volatile DurationSpec.LongMillisecondsBound graceful_disconnect_grace_period = new DurationSpec.LongMillisecondsBound(5000);

		AtomicBoolean actionStarted = new AtomicBoolean(false);
		AtomicInteger connectedChannels = new AtomicInteger(channelGroup.size());


		== Solution

		Introduce an in-band signal — `GRACEFUL_DISCONNECT` — so that the server can notify clients (that have subscribed) connection before closing it. This gives drivers time to:

		if (type == Event.Type.GRACEFUL_DISCONNECT && !DatabaseDescriptor.getGracefulDisconnectEnabled())
		return;

Uh oh!

Conversation

AshenScribe commented Jul 22, 2026

Uh oh!

AshenScribe left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

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!

SiyaoIsHiding 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!

AshenScribe commented Aug 15, 2026

Uh oh!

SiyaoIsHiding commented Aug 19, 2026

Uh oh!

AshenScribe commented Aug 19, 2026

Uh oh!

SiyaoIsHiding commented Aug 19, 2026

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!

AshenScribe commented Aug 20, 2026

Uh oh!

SiyaoIsHiding commented Aug 21, 2026

Uh oh!

AshenScribe commented Aug 25, 2026

Uh oh!

AshenScribe commented Aug 25, 2026

Uh oh!

SiyaoIsHiding commented Aug 25, 2026

Uh oh!

AshenScribe commented Aug 25, 2026

Uh oh!

SiyaoIsHiding commented Aug 25, 2026

Uh oh!

SiyaoIsHiding commented Sep 1, 2026

Uh oh!

AshenScribe commented Sep 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SiyaoIsHiding 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!