Improve LedgerUnderreplicationManager#getLedgerToRereplicate()

JIRA: https://issues.apache.org/jira/browse/BOOKKEEPER-749

Reporter: Rakesh R

As per the discussion in BOOKKEEPER-733, LedgerUnderreplicationManager#getLedgerToRereplicate having multiple resposibilities like : getUrLedger + acquireLock. Actually the idea of this JIRA is to make the interface better.

Please see the discussion thread:
https://issues.apache.org/jira/browse/BOOKKEEPER-733?focusedCommentId=13963072&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13963072

### Comments from JIRA

---
*Rakesh R* 2014-04-28T08:55:45.914+0000

Hi [~ikelly], [~fpj],
I've attached initial draft patch where I've introduced two new interfaces in LedgerUnderreplicationManager:
- Iterator getUrLedgers(GenericCallback cb)
- long tryLockLedgerToRereplicate(String urLedger, final GenericCallback cb)

If we agrees on these interfaces, I'll add testcases and remove the existing api #getLedgerToRereplicate() too. Then later the same can be used by BOOKKEEPER-733

Thanks,
Rakesh

---
*Ivan Kelly* 2014-04-28T09:55:47.565+0000

A method taking a callback *and* returning anything but void make no sense. Is it synchronous or async? This should just be synchronous, since for now it will only be used synchronously by the replication worker. BOOKKEEPER-746 already introduces a listLedgersToReplicate() api.

The iterator should be over longs, not strings. ledgers are identified by longs. Likewise for the first parameter for tryLock.

---
*Rakesh R* 2014-04-28T10:21:43.498+0000

oh ok. Here the difference is, I tried to add zk watcher on visited znode hierarchy, so that it would be helpful to get immediate notifications of new_urledger/lock_del etc as earlier. (presently in my patch its doing polling every 10 seconds BACK_OFF period, probably this will be configurable, isn't it?).

---
*Rakesh R* 2014-04-28T10:25:15.764+0000

Adding one more point: Yes the api is synchronous, the intention of the callback argument in the api is for the zookeeper watch notification.

---
*Ivan Kelly* 2014-04-28T10:30:07.249+0000

I would put async notification into separate apis then, like notifyUnderreplicationChange() and notifyLockStatusChange(). The former may be difficult to implement due to nesting though.

---
*Rakesh R* 2014-04-28T10:46:13.793+0000

Hope will define a new listener for this and keeps a single listener reference at LedgerUnderreplicationManager. This will be used by the zookeeper watcher's of listLedgersToReplicate() and tryLockLedgerToRereplicate() api. Am I correct ?

---
*Ivan Kelly* 2014-04-28T11:03:56.802+0000

I would keep them separate. Conceptually they are separate, so the should be separate in the api also. Really locking should have gone into a separate api from the start, it was just convenient to have it in the same place.

---
*Rakesh R* 2014-04-28T11:33:04.521+0000

How about seeing the underreplication listeners like:
{code}
interface LedgerUrListener{
notify();
}
LedgerUrChangeListener implements LedgerUrListener{
void notify(){
//
}
}
LedgerUrLockStatusChangeListener implements LedgerUrListener{
void notify(){
//
}
}
{code}

---
*Ivan Kelly* 2014-04-28T12:16:14.324+0000

What's the reason for having a base interface? Usually, the notification should carry some context information also.

---
*Rakesh R* 2014-04-28T13:10:37.090+0000

bq.What's the reason for having a base interface?
Instead of notifyUnderreplicationChange() and notifyLockStatusChange() flat structure, I thought of seeing in a hierarchy. Feel free to choose good one:)

bq.Usually, the notification should carry some context information also.
In this case, there is no relevance of context information instead for us its a trigger point to enter into next iteration/cycle by calling LedgerUnderreplicationManager#listLedgersToReplicate() to get the possible items. But we can build context info like, UrChange will have its children and urLockStatus will have the urLedger which was attempted to lock.

LedgerUrListener#notify(T t)

---
*Ivan Kelly* 2014-04-28T15:44:14.855+0000

> Instead of notifyUnderreplicationChange() and notifyLockStatusChange() flat structure, I thought of seeing in a hierarchy. Feel free to choose good one

A flat structure is better here. They're no real relation between the two types of notification.

For notifyUnderreplicationChange() there's no real context. For lock status change there should be though. Waiting on any lock change is excessive, we'd get pounded with notifications. A RW should only wait on a single lock if it has gone through its list and sees no more work available.

---
*Rakesh R* 2014-04-28T17:17:59.589+0000

As compared to the earlier, now the plan is to go with polling approach if there is atleast one urledger is available to re-replicate irrespective of lock failures. Will you be able to incorporate the notifyUnderreplicationChange() in BOOKKEEPER-746.

---
*Ivan Kelly* 2014-04-29T11:50:34.540+0000

notifyUnderreplicationChange doesn't fit in BOOKKEEPER-746, so im not going to put it there. Why not review 746 and 746 so tht they can be committed, and then you can implement notifyUnderreplicationChange on top?

---
*Rakesh R* 2014-04-29T11:59:18.843+0000

Thats right. I'll take a look at it..