fix: close underlying connections when MirroringConnection is closed #49

mwalkiewicz · 2021-10-02T18:31:46Z

This change is

prawilny

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 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 249 at r1 (raw file):

  private void closeMirroringConnectionAndWaitForAsyncOperations() throws IOException {
    Log.trace("close()");

Note that it says "close()" in abort()'s logs. I don't know if it's expected.

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

import java.util.List;

public class IOExceptionsList {

Will create an issue for myself to copy that in appropriate places in the 2.x client.

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

import org.junit.runners.JUnit4;

class TestConnection implements Connection {

I guess that needs another issue assigned to me.

prawilny

Reviewable status: 3 of 4 files reviewed, 4 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 215 at r1 (raw file):

  @Override
  public synchronized void abort(String s, Throwable throwable) {

On the second thought: do we really want to have such a copypaste?
Only now did I notice that the arguments to the function on subconnections are different and that we use Java 7 here (no optionals) and thus the method unifying those two functions would need some hack with nulls denoting no exceptions in close() and ugly lambdas, So I think the copypasta is okayish.

prawilny

Reviewable status: 1 of 5 files reviewed, 7 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 185 at r2 (raw file):

close()

why not getAndSet?

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 225 at r2 (raw file):

        this.mirroringTracer.spanFactory.operationScope(
            HBaseOperation.MIRRORING_CONNECTION_ABORT)) {
      if (this.aborted.get()) {

getAndSet?

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 255 at r2 (raw file):

  private void closeMirroringConnectionAndWaitForAsyncOperations() throws InterruptedException {
    this.referenceCounter.decrementReferenceCount();

I just started to wonder if we shouldn't throw a "stronger" exception below (I think this get() cannot throw an exception, but if it ever did, our reference counting would fail - the connection would be closed too early)

mwalkiewicz

Reviewable status: 1 of 5 files reviewed, 7 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 249 at r1 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

Note that it says "close()" in abort()'s logs. I don't know if it's expected.

Done.

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 185 at r2 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

close()
why not getAndSet?

I've already made this change in one of PR coming after this one, it is not needed now because the method is synchronized.

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 225 at r2 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

getAndSet?

ditto

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 255 at r2 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

I just started to wonder if we shouldn't throw a "stronger" exception below (I think this get() cannot throw an exception, but if it ever did, our reference counting would fail - the connection would be closed too early)

I don't know what do you mean by "stronger". You mean "checked"? If so, how would you want to handle this exception in a calling method?

prawilny

Reviewable status: 1 of 5 files reviewed, 2 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 255 at r2 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

I don't know what do you mean by "stronger". You mean "checked"? If so, how would you want to handle this exception in a calling method?

Oh, I thought that RuntimeException was something "small" and that the user may catch it as if it was some simple error (rather that something that breaks an invariant) and discard it.

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

Previously, prawilny (Adam Czajkowski) wrote…

I guess that needs another issue assigned to me.

Done

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

Previously, prawilny (Adam Czajkowski) wrote…

Will create an issue for myself to copy that in appropriate places in the 2.x client.

Done

mwalkiewicz

Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @dopiera 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 215 at r1 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

On the second thought: do we really want to have such a copypaste?
Only now did I notice that the arguments to the function on subconnections are different and that we use Java 7 here (no optionals) and thus the method unifying those two functions would need some hack with nulls denoting no exceptions in close() and ugly lambdas, So I think the copypasta is okayish.

Done.

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 255 at r2 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

Oh, I thought that RuntimeException was something "small" and that the user may catch it as if it was some simple error (rather that something that breaks an invariant) and discard it.

Done.

dopiera

Reviewable status: 1 of 5 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 180 at r3 (raw file):

  @Override
  public synchronized void close() throws IOException {

I'm wondering if we couldn't implement abort() as:

if (!aborted.getAndSet(true)) {
  close();
}

This will intertwine closed and aborted states, but let's be honest - we don't even know what the proper semantic of abort() and close() should look like.

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 190 at r3 (raw file):

      try {
        closeMirroringConnectionAndWaitForAsyncOperations();

I don't thing there's anything wrong with this now, but I think it's bugprone - we're holding the monitor for a potentially significant amount of time. Moreover, if for some weird reason, another thread needs to access this object to complete, we'll deadlock.

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

  Exception exception = null;

  public void add(IOException exception) {

Why not just one override with Throwable?

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

  }

  public void throwException() throws IOException {

How about calling it rethrowIfCaptured? And the other variant rethrowAsRuntimeIfCaptured()?

prawilny

My objections have been addressed.

Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @dopiera, @mwalkiewicz, and @prawilny)

mwalkiewicz

Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @dopiera 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 180 at r3 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

I'm wondering if we couldn't implement abort() as:
if (!aborted.getAndSet(true)) {
  close();
}
This will intertwine closed and aborted states, but let's be honest - we don't even know what the proper semantic of abort() and close() should look like.

And, because we know nothing, I don't want to mess anything up. I'm leaving handling this eerie async method to the libraries and let them handle this case. I see no reason to do otherwise.

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 190 at r3 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

I don't thing there's anything wrong with this now, but I think it's bugprone - we're holding the monitor for a potentially significant amount of time. Moreover, if for some weird reason, another thread needs to access this object to complete, we'll deadlock.

synchronized is removed from this method, and thus this problem is fixed, in the very next PR that was already approved, but will be merged after this PR.

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

Previously, dopiera (Marek Dopiera) wrote…

Why not just one override with Throwable?

I want to be sure that only IOExceptions and RuntimeExceptions are stored in this object.

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

Previously, dopiera (Marek Dopiera) wrote…

How about calling it rethrowIfCaptured? And the other variant rethrowAsRuntimeIfCaptured()?

Done.

dopiera

Reviewable status: 1 of 7 files reviewed, 5 unresolved discussions (waiting on @dopiera and @prawilny)

…49)

* Initial implementation of hbase1.x Mirroring Client (#1) * Initial implementation of hbase1.x Mirroring Client * MirroringTable and AsyncTableWrapper (#4) * MirroringTable and AsyncTableWrapper - Initial implementation of MirroringTable. - Wrapper around HBase 1.x Table enabling scheduling asynchronous operations. - Asynchronous verification of get/exists results. * Add missing licence header to TestResultComparator (#6) * Setup running tests in GitHub Actions (#7) * Asynchronous ResultScanner (#5) * Asynchronous ResultScanner * Implement faillog.Appender (#2) `faillog.Appender` is the lowermost component of the infrastructure for dumping failed mutations in the `MirroringClient`. `faillog/README.md` explains the design decisions in a bit more detail. `faillog.Appender` essentially logs arbitrary data asynchronously, through a separate thread reading from a bounded buffer. * ListenableReferenceCounter (#8) * Count references in asynchronous tasks before closing Table/Scanner. * Flow controller (#10) * Flow Controller * Flow Control strategy based on single requests queue * Mirroring table: writes (#12) * Add missing condition in result comparator (#15) * Add more mirroring config opitons (#16) * MismatchDetector implementation. * FlowControllerStrategy implementation. * Maximal number of outstanding requests used by FlowControllerStrategy. * Primary/Secondary connection implementation option now accepts "default" value which can be used when default HBase Connection implementation should be used by MirroringConnection. * Make AsyncTableWrapper ListenableCloseable (#20) Implements our standard interface for objects that can run callbacks after asynchronous close to simplify reference counting of MirroringTable. * MirroringConnection: use new config options (#17) * MirroringResultScanner improvements (#18) * Count references to MirroringResultScanner Current implementation of MirroringResultScanner doesn't count verificaiton requests that it has scheduled and allows to close instances while verificaiton requests are in-flight. This causes lost verifications. This PR fixes this issue by counting references to MirroringResultScanner instances when scheduling verification requests. Moreover, ListenableClosable interface is implemented for consistency with other classes that use this scheme, because now the MirroringResultScanner instances will be closed asynchronously, when all scheduled requests have finished. * MirroringTable: count references to scanners and secondary wrapper (#21) Current MirroringTable implementaion does not count its references held by MirroringResultScanners and SecondaryAsyncWrapper, thus MirroringConnection consideres it closed before we are sure that all asynchronous operations have completed. This PR adds correct reference counting of MirroringTable based on work done in previously merged PRs. * Result Scanner - ensure verification ordering (#22) Current implementation assumes that next() operations on primary and secondary scanners are called in the same order and uses this assumption to match results for verification. However, next()s on secondary database are called asynchronusly and their order is not defined, which causes invalid mismatch reports. This PR fixes this problem by placing data to be verified - results of next()s called on primary scanner - and details of next() call - number of requested elements - call on a queue. Each asynchronous call to next() is synchonized and pops a single element from that queue. Appropriate next is called based on number of requested elements. Then results of that request and results from the queue are verified. This ensures that results of next()s passed to verification are correctly matched and ordered. * Integration tests (#23) HBase 1.x integration tests * Add trace logging. (#24) * Estimate memory overhead in RequestResourceDescription (#25) * Tests: extract executor service to TestRule (#26) Extract executor service utilities into TestRule to facilitate code reuse in other test classes. * Integration tests: read configuration from xml files * MirroringBufferedMutator * MirroringBufferedMutator: integration tests (#9) * Fix error introduced in rebase (#11) * Obtain resource reservation before scheduling secondary calls (#4) Fixes a bug when secondary database request Future was created before obtaining resources from FlowController. * Integration tests - MirroringTable operations (#10) * MirroringAsyncConfiguration (#5) Add configuration class to be used by MirroringAsyncConnection. * SecondaryWriteErrorConsumer in MirroringTable (#15) Use SecondaryWriteErrorConsumer to handle write errors in secondary database in MirroringTable's writes. * Use Put to implement Increment and Append (#16) * refactor: extract functions using reflection into package utils.reflection * refactor: extract BatchHelpers into utils Extract common part of batch() helpers into a class and add Predicate argument to nested classes' constructors making it possible to reuse the code in 2.x client. * feat: Initial implementation of a subset of asynchronous HBase 2.x Mirroring Client Contains basic implementation of MirroringAsyncConnection and MirroringAsyncTable. * refactor: extract FlowController's request cancellation into a method * fix: Increment ITs fail with Bigtable as primary (#21) We were setting timerange on Increment objects used in integration tests without any reason and Bigtable doesn't support this operation. Setting timerange in ITs was removed. * fix: RequestScheduling should handle rejected resource reservations (#24) Custom FlowControlerStrategy implementations might, contrary to the default implementation, resolve reservation requests with exception, what we should handle by not performing the action that had to acquire the resources. * feat: Add OpenCensus tracing and metrics. (#14) * fix: make BatchHelpers skip verification of empty read results BatchHelpers provides error handling of batch() when there may be some partial results. Before the commit, matching successful reads were redundantly verified if there were none of them. This commit brings back the behaviour from up to 5a29253: when there are no successful matching reads, a MismatchDetector isn't called on empty arrays. * refactor: make MirroringAsync{Connection,Table} use SecondaryWriteErrorConsumerWithMetrics BatchHelpers require using SecondaryWriteErrorConsumerWithMetrics API. * refactor: make AsyncRequestScheduling accept CompletableFuture<ResourceReservation> instead of ResourceReservation This change is split off from commit introducing MirroringAsyncTable#batch() * feat: implement batch() in MirroringAsyncTable Implementation of MirroringAsyncTable's batch() and MirroringAsyncTable's methods such as get(List<Get) and put(List<Put>) using it. * feat: implement failed mutations log (#19) Failed secondary mutations are written to disk in JSON format, which the user can parse programmatically or inspect visually. Each failure is logged as a separate line, which makes it compatible with solutions like logstash. * refactor: split SplitBatchResponse (#40) SplitBatchResponse was refactored into two parts: splitting into reads/writes and failed/successful. This makes the code simpler and easier to maintain. * refactor: extract helper methods in tests (#48) * refactor: remove redundant writeWithControlFlow argument * feat: copy HBase operations' input lists (#57) * refactor: remove redundant field from MirroringConnection (#55) * feat: verification with sampling (#28) * fix: mirror Increment/Append in batch() using Put. (#47) * refactor: Move HBaseOperation into WriteOperationInfo (#68) * refactor: remove redundant parameter from scheduleWriteWithControlFlow (#69) * fix: integration tests - fix build (#70) * fix: count references to batch operations (#63) * fix: close underlying connections when MirroringConnection is closed (#49) * refactor: fix IDE warnings in MirroringAsyncTable test (#64) * fix: integration tests - check if write errors were reported (#71) * feat: make SecondaryWriteErrorConsumer accept error cause and operation (#65) * fix: do not call callbacks with lock held (#53) * refactor: use AccumulatedExceptions where appropriate (#54) * fix: fix key used in verification sampling ITs (#77) * feat: use faillog for handling write errors (#66) * refactor: add utilities for Futures (FutureUtils) * feat: defer closing connections until async operations complete (#37) Mirroring client schedules asynchronous operations - to mirror the mutations and to verify reads. Before this PR, closing the MirroringAsyncConnection would result in closing the underlying connections immediately. This made the pending asynchronous operations fail. This PR defers closing the underlying connections until all pending operations complete. It is achieved by reference counting the operations. * feat: implement AsyncTableBuilder (#42) * feat: implement MirroringAsyncTable#checkAndMutate (#43) * fix: Implement single Append and Increment as Put (#38) * refactor: simplify SecondaryWriteErrorConsumer API (#78) * feat: concurrent writes in MirroringTable (#79) * test: fix failing concurrent write test (#120) * refactor: renames and moves in RequestScheduling (#87) * wip: handover session comments Co-authored-by: Mateusz Walkiewicz <mwalkiewicz@unoperate.com> Co-authored-by: Adam Czajkowski <prawilny@unoperate.com>

This was linked to issues Oct 2, 2021

Connection not closed in MirroringConnection #29

Closed

Implement isClosed() in MirroringConnection. #33

Closed

prawilny approved these changes Oct 5, 2021

View reviewed changes

prawilny previously approved these changes Oct 5, 2021

View reviewed changes

mwalkiewicz dismissed prawilny’s stale review via 6b003cd October 5, 2021 13:24

mwalkiewicz force-pushed the mw/fix-closing-mirroring-connection/1 branch 3 times, most recently from dee1980 to 496a0cc Compare October 5, 2021 14:44

prawilny requested changes Oct 5, 2021

View reviewed changes

mwalkiewicz commented Oct 8, 2021

View reviewed changes

prawilny previously approved these changes Oct 8, 2021

View reviewed changes

mwalkiewicz requested a review from dopiera October 8, 2021 13:05

mwalkiewicz dismissed prawilny’s stale review via d5326e7 October 8, 2021 13:10

mwalkiewicz force-pushed the mw/fix-closing-mirroring-connection/1 branch from 496a0cc to d5326e7 Compare October 8, 2021 13:10

mwalkiewicz commented Oct 8, 2021

View reviewed changes

mwalkiewicz requested a review from prawilny October 11, 2021 05:57

dopiera requested changes Oct 11, 2021

View reviewed changes

prawilny reviewed Oct 12, 2021

View reviewed changes

mwalkiewicz commented Oct 12, 2021

View reviewed changes

mwalkiewicz requested a review from dopiera October 12, 2021 12:05

mwalkiewicz force-pushed the mw/fix-closing-mirroring-connection/1 branch from 4a3eb14 to b9b639c Compare October 12, 2021 13:08

fix: close underlying connections when MirroringConnection is closed

6c95d87

mwalkiewicz force-pushed the mw/fix-closing-mirroring-connection/1 branch from b9b639c to 6c95d87 Compare October 12, 2021 13:21

dopiera approved these changes Oct 12, 2021

View reviewed changes

mwalkiewicz merged commit 2ae0b79 into master Oct 12, 2021

dopiera pushed a commit that referenced this pull request May 11, 2022

fix: close underlying connections when MirroringConnection is closed (#…

81ff3ce

…49)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: close underlying connections when MirroringConnection is closed #49

fix: close underlying connections when MirroringConnection is closed #49

Uh oh!

mwalkiewicz commented Oct 2, 2021 •

edited by dopiera

Loading

Uh oh!

prawilny left a comment

Uh oh!

prawilny left a comment

Uh oh!

prawilny left a comment

Uh oh!

mwalkiewicz left a comment

Uh oh!

prawilny left a comment

Uh oh!

mwalkiewicz left a comment

Uh oh!

dopiera left a comment

Uh oh!

prawilny left a comment

Uh oh!

mwalkiewicz left a comment

Uh oh!

dopiera left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: close underlying connections when MirroringConnection is closed #49

fix: close underlying connections when MirroringConnection is closed #49

Uh oh!

Conversation

mwalkiewicz commented Oct 2, 2021 • edited by dopiera Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prawilny left a comment

Choose a reason for hiding this comment

Uh oh!

prawilny left a comment

Choose a reason for hiding this comment

Uh oh!

prawilny left a comment

Choose a reason for hiding this comment

Uh oh!

mwalkiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

prawilny left a comment

Choose a reason for hiding this comment

Uh oh!

mwalkiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

dopiera left a comment

Choose a reason for hiding this comment

Uh oh!

prawilny left a comment

Choose a reason for hiding this comment

Uh oh!

mwalkiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

dopiera left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mwalkiewicz commented Oct 2, 2021 •

edited by dopiera

Loading