[jira] [Commented] (BOOKKEEPER-220) Managed Ledger proposal

["Ivan Kelly (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/BOOKKEEPER-220?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409586#comment-13409586 ] 

Ivan Kelly commented on BOOKKEEPER-220:
---------------------------------------

Sorry about taking so long to get around to this. Here's some more comments. I think the ManagedCursor interface is good now. I hadn't really thought through the iterator stuff before suggesting it.
Do you think they should go into bookkeeper-server or as a separate module, bookkeeper-mledger? I'm inclined to think the latter.

** General
- Couple of autogenerated empty javadocs lying around. Best to remove if they add nothing.
- Since these are a lot of new APIs, and guava is being added a dependency, we should mark all public interfaces as com.google.common.annotations.Beta.
- What happens if one reader marks delete on a position, which another reader on the same ledger hasn't read yet?
- You should run findbugs on the module. mvn findbugs:findbugs

** AsyncCallbacks.java
- The callbacks method should be split into success methods and fail methods. It may even make things easier if you just create one callback type using Generics. 
{code}
interface ManagedLedgerCallback<T> {
    void operationComplete(T result, Object ctx);
    void operationFailed(ManagedLedgerException e, Object ctx);
}
{code}

** Position.java 
- javadoc shouldn't mention ledgerId
- It would be nice if toString() printed the name of the managed ledger also.

** ManagedLedgerFactoryImpl.java
- there's a possible race in open(). If two threads try to open the same managed ledger at the same time, there's a time period between the successful thread putting the mledger in the map and initializing the mledger, where any call by the client to the addEntry will hit an assert.
  1. Thread A create ManagedLedgerImpl, puts in map.
  2. Thread B create ManagedLedgerImpl, finds ThreadA's mledger in map. Returns to user.
  3. Thread B calls addEntry on mledger *ASSERT*
- I think the implementation would be cleaner without using executor. It would also solve this race. I'll have a go, see if I can remove the executor.

** ManagedLedgerImpl.java
- You probably don't want people to be able to subclass ManagedManagerImpl, change to the protected methods to package private.

** MetaStoreImplZookeeper.java
- It would be better to use something like protobuf for encode the data stored in zookeeper.


                
> Managed Ledger proposal
> -----------------------
>
>                 Key: BOOKKEEPER-220
>                 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-220
>             Project: Bookkeeper
>          Issue Type: New Feature
>          Components: bookkeeper-client
>            Reporter: Matteo Merli
>            Assignee: Matteo Merli
>             Fix For: 4.2.0
>
>         Attachments: 0001-BOOKKEEPER-220-Managed-Ledger-proposal.patch, 0001-BOOKKEEPER-220-Managed-Ledger-proposal.patch, 0001-BOOKKEEPER-220-Managed-Ledger-proposal.patch
>
>
> The ManagedLedger design is based on our need to manage a set of ledgers, with a single writer (at any point in time) and a set on consumers that read entries from it. 
> The ManagedLedger also takes care of periodically closing ledgers to have a "reasonable" sized sets of ledgers that can individually deleted when no more needed.
> I've put on github the interface proposal (along with an early WIP implementation)
> http://github.com/merlimat/managed-ledger

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira