Exclude defaultrack bookies when enforceMinNumRacksPerWriteQuorum is enabled #1941

reddycharan · 2019-02-08T01:23:14Z

Descriptions of the changes in this PR:

enforceMinNumRacksPerWriteQuorum is meant to be used for strict placement policy. So when
it is enabled, bookies which belong to default faultzone/rack (because of failure in resolving
network location) should be excluded from bookie selection.
add gauge for number of bookies in default faultzone/rack. It will be helpful to create alerts
based on this gauge.
add gauge for number of ledgers found not adhering to strict placement policy in Auditor's
placement policy check. This gauge will be more helpful in creating alert instead of using
monotonously increasing alert.

…enabled - enforceMinNumRacksPerWriteQuorum is meant to be used for strict placement policy. So when it is enabled, bookies which belong to default faultzone/rack (because of failure in resolving network location) should be excluded from bookie selection. - add gauge for number of bookies in default faultzone/rack. It will be helpful to create alerts based on this gauge. - add gauge for number of ledgers found not adhering to strict placement policy in Auditor's placement policy check. This gauge will be more helpful in creating alert instead of using monotonously increasing alert.

reddycharan · 2019-02-08T01:27:21Z

@pasha-kuznetsov fyi.

pasha-kuznetsov · 2019-02-08T19:37:16Z

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

@@ -93,6 +93,9 @@
    String FAILED_CONNECTION_COUNTER = "FAILED_CONNECTION_COUNTER";
    String FAILED_TLS_HANDSHAKE_COUNTER = "FAILED_TLS_HANDSHAKE_COUNTER";

+    // placementpolicy stats
+    String NUM_BOOKIES_IN_DEFAULT_FAULTZONE = "NUM_BOOKIES_IN_DEFAULT_FAULTZONE";


@reddycharan @jvrao Shouldn't we have similar stats for each fault zone, including number of writable bookies there? Then we could alert on a fault zone not having enough (writable) bookies in a fault zone.

responded already - #1941 (comment)

pasha-kuznetsov · 2019-02-08T19:41:49Z

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

+                }
+            }
+        };
+        this.statsLogger.registerGauge(NUM_BOOKIES_IN_DEFAULT_FAULTZONE, numBookiesInDefaultFaultZone);


@reddycharan @jvrao registering the gauge before producing the actual numbers will result in issues similar to #1884 BUG: dir_*_usage stats are reported as 0for read-only bookies after a restart. While the cause is different here, the result will be the same — 0 will always be reported initially after a restart, resetting the alerts (and then reissuing them because it will quickly encounter the same nodes in the default rack)

@pasha-kuznetsov reporting '0' in this case is valid. It means that so far we haven't come across bookie which would fall in default rack. This is appropriate understanding.

pasha-kuznetsov · 2019-02-08T19:47:29Z

...-server/src/test/java/org/apache/bookkeeper/replication/AuditorPlacementPolicyCheckTest.java

-                    placementPolicyCheckEnsembleNotAdheringToPlacementPolicy.get().longValue());
+            Gauge<? extends Number> ledgersNotAdheringToPlacementPolicyGuage = statsLogger
+                    .getGauge(ReplicationStats.NUM_LEDGERS_NOT_ADHERING_TO_PLACEMENT_POLICY);
+            assertEquals("NUM_LEDGERS_NOT_ADHERING_TO_PLACEMENT_POLICY guage value",


will change

sijie

@reddycharan overall looks good to me. I have an additional comment - can we have a stats about num bookies available per rack?

e.g.

/placementpolicy/rack1/num_bookies: 3
/placementpolicy/rack2/num_bookies: 3
...

sijie · 2019-02-09T01:18:01Z

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

+            name = NUM_BOOKIES_IN_DEFAULT_FAULTZONE,
+            help = "Gauge for the number of Bookies in default Fault Zone"
+    )
+    protected Gauge<Integer> numBookiesInDefaultFaultZone;


we rarely use "fault zone" in bookkeeper's concept. I would prefer using a consistent name in the same file, e.g. calling it default rack.

ok will rename it to defaultrack

reddycharan · 2019-02-12T21:54:41Z

@pasha-kuznetsov @sijie i'm little surprised both of you suggested for metrics/stats for num of bookies (writable) available per rack. I have multiple concerns with this

what should be the name of the metric for this stat? It has to be different metric/stat for each rack so different names for each metric/stat. Moreover new Bookies can be added/removed to the cluster so racks can be added/deleted to the cluster. Effectively name of these metrics should be dynamical. If that is the case how can we create actionable alerts without knowing name of the metric/stats upfront?
I can see it might serve as info. purpose but creating an alert effectively for each rack is overkill. For. eg getting an alert saying that there is no writable bookie in a rack of 3 nodes, when there are multiple racks with enough writable bookies is not helpful.
In any case, even if we decide to have such metrics, I feel it should be treated as a separate work item. Because it needs new code to maintain the specific rack details and corresponding stats/metrics (dynamic in nature).

sijie · 2019-02-13T01:47:24Z

what should be the name of the metric for this stat?

I am thinking of placementpolicy/<rack>/num_writable_bookie.

If that is the case how can we create actionable alerts without knowing name of the metric/stats upfront?

different metrics/altering system might have different behaviors. some of the alerting systems are allowed '' queries. so you can write alert rules on using placementpolicy/*/num_writable_bookie (e.g. count("placementpolicy//num_writble_bookies") => return the number of racks)

2. I can see it might serve as info. purpose

I agree with this.

3. In any case, even if we decide to have such metrics, I feel it should be treated as a separate work item.

agree

3. Because it needs new code to maintain the specific rack details and corresponding stats/metrics (dynamic in nature).

in current stats library, you can use scope("rack") for this.

…enabled - testcases

reddycharan · 2019-02-15T06:17:33Z

rerun bookkeeper-server client tests

reddycharan · 2019-02-15T06:18:26Z

@sijie @pasha-kuznetsov @jvrao added testcases.

reddycharan · 2019-02-19T19:24:32Z

@sijie @pasha-kuznetsov @jvrao @eolivelli waiting for final review.

eolivelli · 2019-02-19T20:07:39Z

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

+                if (node instanceof BookieNode) {
+                    BookieNode bookieNode = (BookieNode) node;
+                    comprehensiveExclusionBookiesSet.add(bookieNode.getAddr());
+                }


yeah we should handle else branch.

I think I added this if block just to be on safe side. Leaves of "getDefaultRack()" should be BookieNode. In no normal circumstances it should be non-BookieNode (InnerNode). Anyhow do you have any strong suggestion for what needs to be in else block?

@eolivelli @sijie

No I was only pointing here, maybe something that I missed was missing.

@reddycharan if we don't expect anything otherthan bookie node, we should just remove the if condition. Or add an else with exception. It really doesn't make sense to leave it like that.

will add error logging

sijie · 2019-02-20T00:55:49Z

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

+                if (node instanceof BookieNode) {
+                    BookieNode bookieNode = (BookieNode) node;
+                    comprehensiveExclusionBookiesSet.add(bookieNode.getAddr());
+                }


yeah we should handle else branch.

pasha-kuznetsov

LGTM

reddycharan · 2019-02-20T17:33:49Z

@sijie it should be good to go.

reddycharan · 2019-02-20T21:20:54Z

run bookkeeper-server client tests

jvrao · 2019-02-20T22:43:31Z

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

@@ -251,6 +251,9 @@ EnsemblePlacementPolicy initialize(ClientConfiguration conf,
     * <p>{@code customMetadata} is the same user defined data that user provides
     * when {@link BookKeeper#createLedger(int, int, int, BookKeeper.DigestType, byte[], Map)}.
     *
+     * <p>If 'enforceMinNumRacksPerWriteQuorum' config is enabled then the bookies belonging to default
+     * faultzone (rack) will be excluded while selecting bookies. This is to enable strict placementpolicy.


Not sure if the "This is to enable strict placementpolicy." is needed. Rest of the wording gives enough context.

will remove

jvrao · 2019-02-20T22:50:52Z

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

+            Set<BookieSocketAddress> excludeBookies) {
+        Set<BookieSocketAddress> comprehensiveExclusionBookiesSet;
+        if (enforceMinNumRacksPerWriteQuorum) {
+            comprehensiveExclusionBookiesSet = new HashSet<BookieSocketAddress>(excludeBookies);


Looks like you have put this inside the if condition to avoid unnecessary object allocation. To serve that purpose, may be you should check if the defaultRack is empty or not before allocating new object.

jvrao · 2019-02-20T22:53:02Z

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

+            topology.getLeaves(getDefaultRack()).forEach((Node node) -> {
+                if (node instanceof BookieNode) {
+                    BookieNode bookieNode = (BookieNode) node;
+                    comprehensiveExclusionBookiesSet.add(bookieNode.getAddr());


I would like to have INFO message here with the list of all bookies that are in the default rack and are being excluded. I understand it could amplify logging but I am OK to take it when the configuration is asking for strict placement.

jvrao · 2019-02-20T22:55:56Z

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

-                parentPredicate);
+        rwLock.readLock().lock();
+        try {
+            Set<BookieSocketAddress> comprehensiveExclusionBookiesSet = addDefaultZoneBookiesIfMinNumRacksIsEnforced(


Somehow I am not a big fan of the name. It is too specific to what it is doing. Tomorrow we might add more criteria to exclusion of bookies (Say based on load etc); i am OK with it as-is but why not the name to me "compileComprehensiveExclusionSet" So it is generic; Anyway.. just a suggestion.

dont see an issue. will keep it as it is

jvrao · 2019-02-20T22:58:32Z

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

                     */
                    LOG.warn("Received exception while trying to get network location of bookie: {}", bookie, e);
                }
            }
-            if (racksOrRegionsInQuorum.size() < minNumRacksPerWriteQuorumForThisEnsemble) {
+            if ((racksInQuorum.size() < minNumRacksPerWriteQuorumForThisEnsemble)


Don't you need to check for enforceMinNumRacksPerWriteQuorum for this if condition too? If this is implicit why not the defaultRack is not implicit?

didn't get you, here i'm making sure that number of racks in quorum is not less than minNumRacksPerWriteQuorumForThisEnsemble and if enforceMinNumRacksPerWriteQuorum is enabled then no bookie is from defaultRack.

If enforceMinNumRacksPerWriteQuorum is not enabled, (racksInQuorum.size() < minNumRacksPerWriteQuorumForThisEnsemble) is not an issue right? So I am thinking the check should be enforceMinNumRacksPerWriteQuorum && ((racksInQuorum.size() < minNumRacksPerWriteQuorumForThisEnsemble) || (enforceMinNumRacksPerWriteQuorum && racksInQuorum.contains(getDefaultRack()))

@jvrao If enforceMinNumRacksPerWriteQuorum is not enabled, (racksInQuorum.size() < minNumRacksPerWriteQuorumForThisEnsemble is an issue . for writequorum to adhere to placement policy it should have bookies from atleast minNumRacksPerWriteQuorumForThisEnsemble. if enforceMinNumRacksPerWriteQuorum is enabled then on top of the existing condition, no bookie should be from default rack.

jvrao · 2019-02-21T01:20:52Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java

+                        LOG.error(
+                                "BKAuditException running periodic placementPolicy check."
+                                        + "numOfLedgersNotAdheringToPlacementPolicy {}",
+                                numOfLedgersFoundInPlacementPolicyCheckValue, e);


@reddycharan Why are you checking for non zero value of numOfLedgersFoundInPlacementPolicyCheckValue to set the stat/guage but not for error message? Do you expect to have an exception but a zero numOfLedgersFoundInPlacementPolicyCheckValue?

Do you expect to have an exception but a zero numOfLedgersFoundInPlacementPolicyCheckValue?

yes in the case of exception if we have not found faulty ledgers so far, we don't want to update the gauge value with zero. it would be misleading to report 0. so to avoid false negative i added this if condition in exception case.

I see; what kind of exception do you expect even when numOfLedgersFoundInPlacementPolicyCheckValue is zero.

for placementPolicyCheck, it has to iterate over all the ledgers in the metadataserver and check the placement policy. So half way though the process there could be an exception and no faulty ledger is found so far. So in this case we want to log error but dont want to report gauge with zero value.

…enabled - fixing review comments

reddycharan · 2019-02-21T09:11:48Z

@jvrao addressed your comments.

reddycharan · 2019-02-21T18:00:37Z

rebuild java11

reddycharan · 2019-02-21T18:22:22Z

run bookkeeper-server replication tests

reddycharan · 2019-02-21T18:40:03Z

run bookkeeper-server remaining tests

jvrao

LGTM

reddycharan · 2019-02-21T20:34:31Z

@sijie @jvrao @eolivelli since it is been approved and have successful checks, I'm going to merge this.

reddycharan requested review from sijie and jvrao February 8, 2019 01:25

pasha-kuznetsov reviewed Feb 8, 2019

View reviewed changes

sijie reviewed Feb 9, 2019

View reviewed changes

Exclude defaultrack bookies when enforceMinNumRacksPerWriteQuorum is …

484eb91

…enabled - testcases

reddycharan changed the title ~~(WIP) Exclude defaultrack bookies when enforceMinNumRacksPerWriteQuorum is enabled~~ Exclude defaultrack bookies when enforceMinNumRacksPerWriteQuorum is enabled Feb 15, 2019

eolivelli reviewed Feb 19, 2019

View reviewed changes

sijie approved these changes Feb 20, 2019

View reviewed changes

pasha-kuznetsov approved these changes Feb 20, 2019

View reviewed changes

eolivelli approved these changes Feb 20, 2019

View reviewed changes

jvrao reviewed Feb 20, 2019

View reviewed changes

jvrao reviewed Feb 21, 2019

View reviewed changes

Exclude defaultrack bookies when enforceMinNumRacksPerWriteQuorum is …

c762773

…enabled - fixing review comments

jvrao approved these changes Feb 21, 2019

View reviewed changes

reddycharan added this to the 4.10.0 milestone Feb 21, 2019

reddycharan added area/autorecovery area/client area/stats release/4.10.0 type/improvement labels Feb 21, 2019

reddycharan self-assigned this Feb 21, 2019

reddycharan merged commit 49135b6 into apache:master Feb 21, 2019

This pull request was closed.

Exclude defaultrack bookies when enforceMinNumRacksPerWriteQuorum is enabled #1941

Exclude defaultrack bookies when enforceMinNumRacksPerWriteQuorum is enabled #1941

Conversation

reddycharan commented Feb 8, 2019

reddycharan commented Feb 8, 2019

pasha-kuznetsov Feb 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pasha-kuznetsov Feb 8, 2019 • 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

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

reddycharan commented Feb 12, 2019

sijie commented Feb 13, 2019

reddycharan commented Feb 15, 2019

reddycharan commented Feb 15, 2019 • edited Loading

reddycharan commented Feb 19, 2019

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

pasha-kuznetsov left a comment

Choose a reason for hiding this comment

reddycharan commented Feb 20, 2019

reddycharan commented Feb 20, 2019

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

reddycharan commented Feb 21, 2019

reddycharan commented Feb 21, 2019

reddycharan commented Feb 21, 2019

reddycharan commented Feb 21, 2019

jvrao left a comment

Choose a reason for hiding this comment

reddycharan commented Feb 21, 2019

pasha-kuznetsov Feb 8, 2019 •

edited

Loading

pasha-kuznetsov Feb 8, 2019 •

edited

Loading

reddycharan commented Feb 15, 2019 •

edited

Loading