ISSUE #1390 Ensemble change on delayed write error #1395

jvrao · 2018-05-05T19:08:18Z

Error on delayed writes are dropped if the addEntry
is in complete state (ack quorum satisfied).
This change records the delayed write failure and forces
ensemble change onthe next write. This saves from having
extended under replicated status on the ledger and also
avoids unnecessary build up at PCBC channel.

Signed-off-by: Venkateswararao Jujjuri (JV) vjujjuri@salesforce.com

Descriptions of the changes in this PR:

(PR description content here)...

Master Issue: #

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

Make sure the PR title is formatted like:
<BP-#>: Description of bookkeeper proposal
e.g. BP-1: 64 bits ledger is support

Attach the master issue link in the description of this PR.

Attach the google doc link if the BP is written in Google Doc.

Otherwise:

Make sure the PR title is formatted like:
<Issue #>: Description of pull request
e.g. Issue 123: Description ...

Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.

Replace <Issue #> in the title with the actual Issue number.

Error on delayed writes are dropped if the addEntry is in complete state (ack quorum satisfied). This change records the delayed write failure and forces ensemble change onthe next write. This saves from having extended under replicated status on the ledger and also avoids unnecessary build up at PCBC channel. Signed-off-by: Venkateswararao Jujjuri (JV) <vjujjuri@salesforce.com>

eolivelli

The idea is very smart.
I left some minor comment

I wonder if we should add a config option in order to enable explicitly this change, maybe we could keep it 'disabled' in 4.8 and then change default to 'enable' in 4.9

I also wonder if it is possible to use mockito based client side framework and save us from starting a real cluster in tests. (Not blocker for me)

eolivelli · 2018-05-05T19:20:39Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java

@@ -1749,6 +1755,42 @@ EnsembleInfo replaceBookieInMetadata(final Map<Integer, BookieSocketAddress> fai
        return new EnsembleInfo(newEnsemble, failedBookies, replacedBookies);
    }

+    void handleDelayeWriteBookieFailure() {


eolivelli · 2018-05-05T19:24:33Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java

@@ -151,6 +153,10 @@
        }
    }

+    public Map<Integer, BookieSocketAddress> getDelayedWriteFailedBookies() {


Why do we have to expose as 'public' ?

took care of it.

eolivelli · 2018-05-05T19:26:10Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java

@@ -117,6 +118,7 @@
    ScheduledFuture<?> timeoutFuture = null;

    final long waitForWriteSetMs;
+    Map<Integer, BookieSocketAddress> delayedWriteFailedBookies = new HashMap<Integer, BookieSocketAddress>();


Can you please explain why this is not a ConcurrentMap ?

Because of or ordered safe executor, I thought a regular map is good enough.

I was thinking the same. Just wanted to double check.

jvrao · 2018-05-06T13:57:41Z

I wonder if we should add a config option in order to enable explicitly this change

@eolivelli I thought about it - Felt that it can just follow the same config path we use to make ensemble change on the failed add entry path. This patch still respects delayedEnsemble change and disableEnsembleChange configurations.

jvrao · 2018-05-06T14:00:58Z

I also wonder if it is possible to use mockito based client side framework and save us from starting a real cluster in tests. (Not blocker for me)

Are you making a general comment about tests in this class? if so, maybe we can open an issue and deal with that separately?

eolivelli · 2018-05-06T14:02:06Z

For me it is okay.
I would like to know @sijie 's opinion

eolivelli

+1

eolivelli · 2018-05-06T18:47:38Z

Regarding the test: I did not think that this is not a new class, so okay for keeping your test as it is.

Regarding the map: okay for making it a concurrent map, in this case the map will be very small and rarely used so using a ConcurrentHashMap will not have significant impact and it makes things simpler. It can be a simple hashmap btw

sijie

@jvrao before giving a detailed review, just one general question:

any reason why we can't re-use bookieFailureHistory loading cache?

sijie · 2018-05-07T08:35:13Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java

@@ -117,6 +118,7 @@
    ScheduledFuture<?> timeoutFuture = null;

    final long waitForWriteSetMs;
+    private Map<Integer, BookieSocketAddress> delayedWriteFailedBookies = new HashMap<Integer, BookieSocketAddress>();


nit: final?

Sure, I can do that; but would wait to see if you have any other comments.

jvrao · 2018-05-07T19:22:10Z

any reason why we can't re-use bookieFailureHistory loading cache?

I thought the purpose is different and usage is different. In this case, it the index, not entryId that goes in with bookieIP, also this will be cleared as soon as the ensemble change attempted, and the population of this is based on the configuration parameters based on ensemble change etc etc. Moreover simple to have another field that is not bundled with something else.

sijie

@jvrao looks good to me.

sijie · 2018-05-10T06:43:52Z

retest this please

jvrao · 2018-05-11T18:08:02Z

retest this please

sijie · 2018-05-17T04:01:36Z

retest this please

Descriptions of the changes in this PR: The Original intent of this change is to do a best-effort ensemble change. But this is not possible until the local metadata is completely immutable. Until the feature "Make LedgerMetadata Immutable #610" Is complete we will use handleBookieFailure() to handle delayed writes as regular bookie failures. Signed-off-by: Venkateswararao Jujjuri (JV) <vjujjurisalesforce.com> Master Issue: #1591 Relate Issue: #1395 Author: JV Jujjuri <vjujjuri@salesforce.com> Author: Ivan Kelly <ivank@apache.org> Reviewers: Ivan Kelly <ivank@apache.org>, Sijie Guo <sijie@apache.org> This closes #1592 from jvrao/datalossbug

Descriptions of the changes in this PR: The Original intent of this change is to do a best-effort ensemble change. But this is not possible until the local metadata is completely immutable. Until the feature "Make LedgerMetadata Immutable #610" Is complete we will use handleBookieFailure() to handle delayed writes as regular bookie failures. Signed-off-by: Venkateswararao Jujjuri (JV) <vjujjurisalesforce.com> Master Issue: #1591 Relate Issue: #1395 Author: JV Jujjuri <vjujjuri@salesforce.com> Author: Ivan Kelly <ivank@apache.org> Reviewers: Ivan Kelly <ivank@apache.org>, Sijie Guo <sijie@apache.org> This closes #1592 from jvrao/datalossbug (cherry picked from commit 3ab6e92) Signed-off-by: Ivan Kelly <ivank@apache.org>

@bug

* Avoid releasing sent buffer to early in BookieClient mock If the buffer was sent to more than one bookie with the mock, it would be released after being sent to the first one. Each write should retain a refCount themselves, and then release when done. Author: Ivan Kelly <ivank@apache.org> Reviewers: Sijie Guo <sijie@apache.org> This closes apache#1598 from ivankelly/double-rel-mock * (@bug W-5344681@) Delayed write ensemble change may cause dataloss Descriptions of the changes in this PR: The Original intent of this change is to do a best-effort ensemble change. But this is not possible until the local metadata is completely immutable. Until the feature "Make LedgerMetadata Immutable apache#610" Is complete we will use handleBookieFailure() to handle delayed writes as regular bookie failures. Signed-off-by: Venkateswararao Jujjuri (JV) <vjujjurisalesforce.com> Master Issue: apache#1591 Relate Issue: apache#1395 Author: JV Jujjuri <vjujjuri@salesforce.com> Author: Ivan Kelly <ivank@apache.org> Reviewers: Ivan Kelly <ivank@apache.org>, Sijie Guo <sijie@apache.org> @Rev Sam Just@ This closes apache#1592 from jvrao/datalossbug

jvrao requested a review from sijie May 5, 2018 19:08

eolivelli requested changes May 5, 2018

View reviewed changes

Review comments - 1

3a9be54

eolivelli approved these changes May 6, 2018

View reviewed changes

sijie reviewed May 7, 2018

View reviewed changes

sijie assigned jvrao May 7, 2018

sijie added area/client type/improvement release/4.8.0 labels May 7, 2018

sijie added this to the 4.8.0 milestone May 7, 2018

sijie approved these changes May 8, 2018

View reviewed changes

Review-2

f8a6fc0

sijie closed this in a267478 May 17, 2018

sijie mentioned this pull request Aug 9, 2018

Deferred failure handling can cause data loss #1591

Closed

jvrao mentioned this pull request Aug 9, 2018

Delayed write ensemble change may cause dataloss #1592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE #1390 Ensemble change on delayed write error #1395

ISSUE #1390 Ensemble change on delayed write error #1395

jvrao commented May 5, 2018

eolivelli left a comment

eolivelli May 5, 2018

jvrao May 6, 2018

eolivelli May 5, 2018

jvrao May 8, 2018

eolivelli May 5, 2018

jvrao May 6, 2018

eolivelli May 6, 2018

jvrao commented May 6, 2018 •

edited

Loading

jvrao commented May 6, 2018

eolivelli commented May 6, 2018

eolivelli left a comment

eolivelli commented May 6, 2018

sijie left a comment

sijie May 7, 2018

jvrao May 7, 2018

jvrao commented May 7, 2018 •

edited

Loading

sijie left a comment

sijie commented May 10, 2018

jvrao commented May 11, 2018

sijie commented May 17, 2018

ISSUE #1390 Ensemble change on delayed write error #1395

ISSUE #1390 Ensemble change on delayed write error #1395

Conversation

jvrao commented May 5, 2018

eolivelli 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

jvrao commented May 6, 2018 • edited Loading

jvrao commented May 6, 2018

eolivelli commented May 6, 2018

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli commented May 6, 2018

sijie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvrao commented May 7, 2018 • edited Loading

sijie left a comment

Choose a reason for hiding this comment

sijie commented May 10, 2018

jvrao commented May 11, 2018

sijie commented May 17, 2018

jvrao commented May 6, 2018 •

edited

Loading

jvrao commented May 7, 2018 •

edited

Loading