feat: MirroringConnection: timeout in close() and abort() #133

prawilny · 2021-11-03T10:11:58Z

This change is

TestBlocking#testConnectionCloseBlocksUntilAllRequestsHaveBeenVerified doesn't pass - also on master.

mwalkiewicz

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @mwalkiewicz and @prawilny)

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringConnection.java, line 283 at r1 (raw file):

  private void terminateConnectionsWithTimeout(
      final Callable<Void> terminatingAction, boolean throwExceptionIfErred) throws IOException {

There is something called CallableThrowingIOException in our project, we can use it here to make exceptions thrown by terminatingAction more specific.

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringConnection.java, line 295 at r1 (raw file):

      } catch (Exception e) {
        if (throwExceptionIfErred) {
          throw new IOException(e);

Can't we throw this exception unconditionally and just catch and ignore it in abort? We are already ignoring something there.

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringConnection.java, line 301 at r1 (raw file):

      assert false;
    } catch (InterruptedException e) {
      throw new RuntimeException(e);

Why won't we throw a IOException(e)?

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringConnection.java, line 302 at r1 (raw file):

    } catch (InterruptedException e) {
      throw new RuntimeException(e);
    } catch (TimeoutException e) {

I think that we should an error to a logger (to stdout) here.

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringConnection.java, line 302 at r1 (raw file):

    } catch (InterruptedException e) {
      throw new RuntimeException(e);
    } catch (TimeoutException e) {

So, if the timeout happens, the connections are not closed and the user is not notified. I think that we can throw a IOException(e) after scheduling the terminatingAction and let the user know that there is something went wrong (because the primary was not closed).

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/MirroringConfigurationHelper.java, line 136 at r1 (raw file):

   * pending operations before terminating connections with primary and secondary databases.
   *
   * <p>Defaults to 1000. If the wait times out, the termination is scheduled asynchronously.

I'd set the default to 60 seconds, we really want to wait for all the writes to finish.

prawilny

Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @mwalkiewicz)

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringConnection.java, line 283 at r1 (raw file):