Issue 709: Add Slow Bookkeeper Servers to Placement Policy for read ordering

[philipsu522]

Descriptions of the changes in this PR:

Maintain a list of slow bookkeeper hosts, which are tried only after readonly bookkeeper hosts are tried. Bookkeeper hosts can be categorized as "slow" when a speculative timeout occurs on that bookkeeper host.

[eolivelli]

I like the idea of adding a bean instead of raw collections.
Averall lokks good to me, but I left some minor comments

[eolivelli]

can't we preserve old value for constant ?

[ivankelly]

No, the order is important, since candidate bookies are sorted by these masks. You want to select a slow bookie before an unavailable one. You probably want to select a slow bookie before a readonly bookie too.

[eolivelli]

Ok. I understand

[eolivelli]

nit: remove empty line

[eolivelli]

nit: testNodeSlow

[eolivelli]

please make this and other getters public, placement policies are pluggable, so if you pass an object to a policy it should be able to "use" it

@sijie @ivankelly

[eolivelli]

this class provides info about multiple bookies.
what about "BookiesHealthInfo" ?

[sijie]

@eolivelli If naming is not a big concern, let's skip it or rename it later, since it is a port back from twitter branch.

I think it is good to make this class an interface, since it will be used and exposed in EnsemblePlacementPolicy. we should do similar thing as DistributedSchedule.WriteSet. However, preserve the naming here and do a separate change to make it an interface, it will make change porting easier for them.

[eolivelli]

Ok I didn't know it is a port. Maybe it was not clear from the description.
+1

[eolivelli]

Ok I didn't know it is a port. Maybe it was not clear from the description.
+1

[philipsu522]

I could make this change - it wasn't a clean port so I had to reimplement a lot of stuff anyways.

[jiazhai]

This is a great idea.
A question in my mind is that, How it comes when a bookie is first slow, but then after a fix or something it becomes fast?

[eolivelli]

+1 lgtm, we can improve code in later changes

[ivankelly]

@philipsu522 @sijie

Maintain a list of slow bookkeeper hosts, which are tried only after readonly bookkeeper hosts are tried.

What is the rationale for trying read-only before slow? Surely a write to the read-only is likely to fail.

[ivankelly]

eid? I don't see this defined in the local scope.

[philipsu522]

Not sure why it's showing "eId" when the change looks like it says "entryId" now. In any case, this value defined in the super class LedgerEntry:

    abstract class LedgerEntryRequest extends LedgerEntry implements SpeculativeRequestExecutor {

and

public class LedgerEntry
    implements org.apache.bookkeeper.client.api.LedgerEntry {

    final long ledgerId;
    long entryId;
    long length;
    ByteBuf data;

[philipsu522]

Actually looks like the LedgerEntry interface was changed in a recent commit - I'll need a bit more time to incorporate that change. Will submit changes soon.

[ivankelly]

👍

[ivankelly]

broken indent

[ivankelly]

i don't think we need to check if this is enabled here. if disabled, the placement policy should just ignore the extra info.

[ivankelly]

We can probably do this without autoboxing, but not something to worry about for this change.

[philipsu522]

fixed

[ivankelly]

yikes, this is going to run for each entry and allocates a map. Why not use getIfPresent?

[ivankelly]

hmm, maybe not. Still though, getIfPresent is more direct

[sijie]

I don't think there is getIfPresent in LoadingCache. If I am correct, the only way to get a key from loading cache without loading, is asMap().containsKey(address)).

[philipsu522]

@sijie is correct - there isn't a getIfPresent() function for LoadingCaches. Another option is to call get() and if it returns -1 (defined by the LoadingCache constructor), we can assume it doesn't exist. Thoughts on this?

[sijie]

I think both get and getUnchecked do loading the key. we don't need loading a key here, so I think we should stick to asMap().containsKey(address) here.

[ivankelly]

getIfPresent is part of Cache, which LoadingCache inherits from.
https://google.github.io/guava/releases/20.0/api/docs/com/google/common/cache/LoadingCache.html

[philipsu522]

fixed in latest commit

[ivankelly]

why is all this region stuff going into rack aware placement policy? Shouldn't it be region aware?

[philipsu522]

That's a good question. The reasoning is that I wanted to try to avoid duplication in RackAware and RegionAware placement policies, so I put the shared code in RackAwarePlacementPolicyImpl so that both of the policies can access it since RegionAware is a sublcass of RackAware.

[ivankelly]

Ok. We should probably refactor this at some point. methods with loads of parameters and boolean switches become very hard to follow and maintain.

[sijie]

What is the rationale for trying read-only before slow? Surely a write to the read-only is likely to fail.

I am not sure about. I think slow should be before read-only. @philipsu522 @yzang can you guys answer this?

[philipsu522]

This is a great idea.
A question in my mind is that, How it comes when a bookie is first slow, but then after a fix or something it becomes fast?

@jiazhai, the loading cache will evict the slow bookie after a certain amount of time based on last write , so if it becomes fast then there won't be any speculative reads. No more speculative reads means that the cache won't be updated for the bookie, and it will be evicted after a certain time.

[philipsu522]

I am not sure about. I think slow should be before read-only. @philipsu522 @yzang can you guys answer this?

I think that we put slow bookies behind read-only bookies thinking that slow bookies are more likely to hurt performance, while bookies could be in RO for any number of reasons. However, based on experience of everyone else, if RO bookies seem to be less reliable than slow bookies, I don't mind changing the order so that it is available bookies, slow bookies, RO bookies...

What is the rationale for trying read-only before slow? Surely a write to the read-only is likely to fail.

I believe that the new feature in placement policy is used for reads, not writes

[ivankelly]

@philipsu522 re: slow vs read only, makes sense. I had it in my head that this was used for writes, but reordering only happens for reads.

[ivankelly]

@philipsu522 could you merge master into this, and address any comments when you have a chance. If it's left too long it'll be a nightmare to get in.

[philipsu522]

Yep, sorry about that - I'll get this done today

[sijie]

LGTM +1

@philipsu522 can you help me rebase this to latest master?

[philipsu522]

@sijie I just rebased master

[ivankelly]

Duplicate code from method above.

[ivankelly]

This is very allocatey for something that runs for every read request. It allocates a new map for bookiePendingMap, and converts bookieFailureHistory into a map, and allocates a new BookiesHealthInfo object, but all of these are just references to info elsewhere in the client.

Instead of this, we can have BookiesHealthInfo as a interface, and have a member field in LedgerHandle or BookKeeper.

final BookiesHealthInfo bookieHealthInfo = new BookiesHealthInfo() {
    @Override
    public long getBookieFailureHistory(BookieSocketAddress bookieSocketAddress) {
        Long lastFailure = bookieFailureHistory.getIfPresent(bookieSocketAddress);
        return lastFailure == null ? -1L : lastFailure;
    }

    @Override
    public int getBookiePendingRequests(BookieSocketAddress bookieSocketAddress) {
        PerChannelBookieClientPool pcbcPool = client.lookupClient(address);
        return pcbcPool == null ? 0 : pcbcPool.getNumPendingCompletionRequests();
   }
}

[philipsu522]

@ivankelly makes sense - pushed a change to convert BookiesHealthInfo into an interface

[sijie]

@ivankelly can you review this PR again after @philipsu522 addressed your comments?

[ivankelly]

@sijie @philipsu522 I'll take a look first thing tomorrow. Got caught up on other things today.

@philipsu522 Seems this needs another merge from master. I guess it's checkstyle changes, so the easiest thing to do would be take your changes in the conflict resolution, compile and fix any issues again.

[ivankelly]

@philipsu522 lgtm. Just needs to be updated to master before merging in.