BOOKKEEPER-950: Ledger placement policy to accomodate different storage capacity of bookies by rithin-shetty · Pull Request #93 · apache/bookkeeper

rithin-shetty · 2016-12-14T19:41:44Z

…ge capacity of bookies

This change introduces Disk weight based ledger placement. Currently free disk space is the only supported
weight for a bookie. This change also introduces a new protocol message between bk client and server
called GET_BOOKIE_INFO. This message is used by the client to retrieve the free disk space info from
all the bookies. The existing placement policies: DefaultPlacementPolicy and RackAwarePlacementPolicy
have been enhanced to make use of the weight while selecting bookies. New test cases have been added to
test RackawarePlacement with weights. A new test class has been added to test the weight based selection
algorithm in a stand alone fashion.

sijie · 2016-12-20T21:49:36Z

@rithin-shetty sorry for late response. I will try to get to this one this week.

btw this pull request has conflicts on bookkeeper-server/pom.xml. do you mind rebasing it?

rithin-shetty · 2016-12-20T22:04:14Z

@sijie thanks for pointing that out. I have now rebased and updated the branch. The merge conflict should be gone....

eolivelli

Sorry my review has been pending for days

eolivelli · 2016-12-14T21:09:15Z

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

+     *          A map that has the bookie to BookieInfo
+     */
+    public void updateBookieInfo(Map<BookieSocketAddress, BookieInfo> bookieInfoMap);
 }


Can you add a default noop implementation?

Sure, I'll add a default implementation.

eolivelli · 2016-12-14T21:13:34Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java

    protected final static String BOOKIE_QUARANTINE_TIME_SECONDS = "bookieQuarantineTimeSeconds";

+    // Bookie info poll interval
+    protected final static String DISK_WEIGHT_BASED_PLACEMENT_ENABLED = "diskWeightBasedPlacementEnabled";


Maybe it would be better to distinguish the polling of bookieinfo from the activation of the new behaviour ?
This way from BookKeeper client I will be able to ask the global status of the system without using the new policy

What is your use case ? Do you want to retrieve the status from all the bookies just once or on a periodic basis ? I'd suggest when you extend the usage of this feature, make the necessary adjustments. You'll have a clearer idea of what the requirements are.

You are right, when we will need to have this info for other purposes we will introduce new flags.

My idea was just to be able to ask for bookie status without altering how the placement policy works, but actually I have to valid use case.

sijie

overall looks good to me.

some minor comments and the pr needs to rebase on latest master.

after that, it is ready for merging.

sijie · 2017-01-31T01:31:17Z

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

+    private final AtomicBoolean refreshBookieList = new AtomicBoolean();
+
+    public static class BookieInfo implements WeightedObject {
+        private long freeDiskSpace;


ok. modified...

sijie · 2017-01-31T01:31:25Z

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

+
+    public static class BookieInfo implements WeightedObject {
+        private long freeDiskSpace;
+        private long totalDiskSpace;


ok. made final...

rithin-shetty · 2017-01-31T21:03:46Z

@sijie I've addressed your comments. I've uploaded the merged changes. But the Jenkins build has been failing with the following error for some time now. Do you know how to fix this ? It's very annoying and should be fixed.

[ERROR] Failed to execute goal org.apache.rat:apache-rat-plugin:0.7:check (default-cli) on project bookkeeper-server: Too many unapproved licenses: 2 -> [Help 1]

sijie · 2017-01-31T21:44:27Z

@rithin-shetty I am fixing that by #103

sijie · 2017-01-31T21:48:20Z

LGTM. +1

@eolivelli since you were also reviewing this change, can you take a look at the updated pull request?

eolivelli

One minor comment (see inline comments).

@rithin-shetty @sijie
I think it would be better to add a test case on the timeout on GetBookieInfo, something like testReadTimeout

eolivelli · 2017-02-01T07:39:14Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java

+            return lOpts;
+        }
+
+        String getReadable(long val) {


maybe this could be 'static' ?

since this is a nested class, I can't make it static.

sijie · 2017-02-01T18:56:39Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/GetBookieInfoProcessorV3.java

@@ -0,0 +1,71 @@
+package org.apache.bookkeeper.proto;


need an apache license header

ok, will add.

rithin-shetty · 2017-02-02T00:38:16Z

@eolivelli I have added a new test case for testing the timeout of GET_BOOKIE_INFO op. See TestGetBookieInfoTimeout.java

jiazhai · 2017-02-02T03:35:00Z

...keeper-server/src/main/java/org/apache/bookkeeper/client/DefaultEnsemblePlacementPolicy.java

+        try {
            allBookies = new ArrayList<BookieSocketAddress>(knownBookies);
+        } finally {
+            rwLock.readLock().unlock();


Do we need to consider the race that 'knownBookies' is changed between this line and line 79?

Thats should be ok because if we end up choosing a bookie that just went down, the write will fail and we'll come back and change the ensemble. BTW, this could happen even without my change. So I'm not changing the situation any way.

eolivelli · 2017-02-02T16:58:03Z

For me is ok
+1

sijie

LGTM +1. @rithin-shetty Do you mind helping me rebase the pull request? so I can merge it.

…ge capacity of bookies This change introduces Disk weight based ledger placement. Currently free disk space is the only supported weight for a bookie. This change also introduces a new protocol message between bk client and server called GET_BOOKIE_INFO. This message is used by the client to retrieve the free disk space info from all the bookies. The existing placement policies: DefaultPlacementPolicy and RackAwarePlacementPolicy have been enhanced to make use of the weight while selecting bookies. New test cases have been added to test RackawarePlacement with weights. A new test class has been added to test the weight based selection algorithm in a stand alone fashion.

rithin-shetty · 2017-03-23T16:53:12Z

@sijie, I have merged and uploaded the latest diffs.

@Rev

… non-writable channel (apache#93) - Logging is more detailed now. - Ones PCBC fail request because of non-writable channel it will fast fail other requests until channel becomes writable. - addressed cases when checks for channel nullability allowed null channel later if updated concurrently (@Rev vjujjuri@)

rithin-shetty force-pushed the weightBasedPlacementDec13 branch from e78b11d to 745f3db Compare December 20, 2016 22:02

rithin-shetty force-pushed the weightBasedPlacementDec13 branch from 745f3db to 5744d53 Compare December 21, 2016 18:10

eolivelli requested changes Dec 28, 2016

View reviewed changes

rithin-shetty force-pushed the weightBasedPlacementDec13 branch from 5744d53 to 46a0a0f Compare January 17, 2017 23:00

rithin-shetty force-pushed the weightBasedPlacementDec13 branch from 46a0a0f to 0c1b185 Compare January 29, 2017 01:28

sijie reviewed Jan 31, 2017

View reviewed changes

rithin-shetty force-pushed the weightBasedPlacementDec13 branch from 0c1b185 to 8c2d5a9 Compare January 31, 2017 21:00

eolivelli requested changes Feb 1, 2017

View reviewed changes

sijie reviewed Feb 1, 2017

View reviewed changes

rithin-shetty force-pushed the weightBasedPlacementDec13 branch from 8c2d5a9 to 3892c2e Compare February 2, 2017 00:35

jiazhai reviewed Feb 2, 2017

View reviewed changes

rithin-shetty force-pushed the weightBasedPlacementDec13 branch from 3892c2e to 675007a Compare February 2, 2017 07:49

rithin-shetty force-pushed the weightBasedPlacementDec13 branch from 675007a to 9a61541 Compare February 14, 2017 19:03

rithin-shetty force-pushed the weightBasedPlacementDec13 branch from 9a61541 to d390900 Compare February 23, 2017 18:18

sijie approved these changes Mar 23, 2017

View reviewed changes

rithin-shetty force-pushed the weightBasedPlacementDec13 branch from d390900 to b411161 Compare March 23, 2017 15:37

rithin-shetty changed the title ~~BOOKKEEPER-950: Ledger placement policy to accomodate different stora…~~ BOOKKEEPER-950: Ledger placement policy to accomodate different storage capacity of bookies Mar 23, 2017

sijie approved these changes Mar 28, 2017

View reviewed changes

asfgit closed this in 0583175 Mar 28, 2017

Conversation

rithin-shetty commented Dec 14, 2016

Uh oh!

sijie commented Dec 20, 2016

Uh oh!

rithin-shetty commented Dec 20, 2016

Uh oh!

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

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

rithin-shetty commented Jan 31, 2017

Uh oh!

sijie commented Jan 31, 2017

Uh oh!

sijie commented Jan 31, 2017

Uh oh!

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

rithin-shetty commented Feb 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Feb 2, 2017

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

rithin-shetty commented Mar 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants