[
https://issues.apache.org/jira/browse/BOOKKEEPER-220?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13409586#comment-13409586
]
Ivan Kelly edited comment on BOOKKEEPER-220 at 7/9/12 3:54 PM:
---------------------------------------------------------------
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.
was (Author: ikelly):
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
