You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Matteo Merli <mm...@yahoo-inc.com> on 2015/04/11 00:18:54 UTC
Review Request 33096: Configurable LedgerStorageImplementation
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33096/
-----------------------------------------------------------
Review request for bookkeeper.
Bugs: BOOKKEEPER-851
https://issues.apache.org/jira/browse/BOOKKEEPER-851
Repository: bookkeeper-git
Description
-------
Allow to configure a different implementation for the LedgerStorage interface in the Bookie configuration.
Diffs
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookKeeperServerStats.java 9036182
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 8b40853
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java 1a69692
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java e992d03
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorageFactory.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ReadOnlyEntryLogger.java a2ce9e3
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java d8a87e4
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java cd9f7a0
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java 6c9c4a7
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/TestSyncThread.java d947dff
bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java 19aab44
Diff: https://reviews.apache.org/r/33096/diff/
Testing
-------
Thanks,
Matteo Merli
Re: Review Request 33096: Configurable LedgerStorageImplementation
Posted by Matteo Merli <mm...@yahoo-inc.com>.
> On May 19, 2015, 7:12 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java, line 498
> > <https://reviews.apache.org/r/33096/diff/1/?file=923970#file923970line498>
> >
> > why not create the sorted ledger storage also by reflection?
I didn't want to break configuration compatibility. Today if you specify sortedLedgerStorageEnabled=true, that means to instantiate a different class. We can deprecate the sortedLedgerStorageEnabled option, or remove it and set the SortedLedgerStorage as the default implementation.
> On May 19, 2015, 7:12 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java, line 122
> > <https://reviews.apache.org/r/33096/diff/1/?file=923971#file923971line122>
> >
> > the comment isn't accurate. it isn't syncing the underlying storage though.
After the compaction is done, all the indexes are updated and they are indeed fsynced before the old entryLog can be deleted, otherwise we might lose the access the compacted entries.
I'll change the comment to "sync the underlying *index*"
> On May 19, 2015, 7:12 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java, line 424
> > <https://reviews.apache.org/r/33096/diff/1/?file=923971#file923971line424>
> >
> > this changes the behavior here. it doesn't need to load file info to file info cache before. but right now, it would, which it would pollute the file info cache and evict other active file infos.
> >
> > you might consider adding a separate interface for CompatableLedgerStorage.
That's true, though I don't see why `IndexPersistenceMgr.ledgerExist()` cannot be simplyfied into:
```
boolean ledgerExists(long ledgerId) throws IOException {
return activeLedgers.containsKey(ledgerId);
}
```
> On May 19, 2015, 7:12 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java, line 245
> > <https://reviews.apache.org/r/33096/diff/1/?file=923971#file923971line245>
> >
> > this seems incorrect. you'd better to have a separate method 'syncEntriesLocations', which differentiate 'updateEntriesLocations' from 'syncEntriesLocations'.
OK, I see your point, this code is actually flushing the indexes multiple times during compaction. Fixed it.
> On May 19, 2015, 7:12 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java, line 85
> > <https://reviews.apache.org/r/33096/diff/1/?file=923976#file923976line85>
> >
> > why change this logic here? It'd better to not to change it if it isn't a problem.
Reverted to original logic
- Matteo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33096/#review84268
-----------------------------------------------------------
On April 10, 2015, 10:18 p.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33096/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 10:18 p.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-851
> https://issues.apache.org/jira/browse/BOOKKEEPER-851
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Allow to configure a different implementation for the LedgerStorage interface in the Bookie configuration.
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookKeeperServerStats.java 9036182
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 8b40853
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java 1a69692
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java e992d03
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorageFactory.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ReadOnlyEntryLogger.java a2ce9e3
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java d8a87e4
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java cd9f7a0
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java 6c9c4a7
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/TestSyncThread.java d947dff
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java 19aab44
>
> Diff: https://reviews.apache.org/r/33096/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>
Re: Review Request 33096: Configurable LedgerStorageImplementation
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33096/#review84268
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
<https://reviews.apache.org/r/33096/#comment135453>
why not create the sorted ledger storage also by reflection?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
<https://reviews.apache.org/r/33096/#comment135451>
could you keep the scope as before?
if you want to introduce a new scope 'storage', you could scope it in the new storage implementation.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
<https://reviews.apache.org/r/33096/#comment135456>
could you move the CompactableLedgerStorage out of GarbageCollectionThread to a separated file in bookie package?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
<https://reviews.apache.org/r/33096/#comment135454>
trailing space?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
<https://reviews.apache.org/r/33096/#comment135458>
the comment isn't accurate. it isn't syncing the underlying storage though.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
<https://reviews.apache.org/r/33096/#comment135455>
trailing space?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
<https://reviews.apache.org/r/33096/#comment135457>
moved this to a separated file under bookie package.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
<https://reviews.apache.org/r/33096/#comment135459>
this seems incorrect. you'd better to have a separate method 'syncEntriesLocations', which differentiate 'updateEntriesLocations' from 'syncEntriesLocations'.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
<https://reviews.apache.org/r/33096/#comment135463>
this changes the behavior here. it doesn't need to load file info to file info cache before. but right now, it would, which it would pollute the file info cache and evict other active file infos.
you might consider adding a separate interface for CompatableLedgerStorage.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java
<https://reviews.apache.org/r/33096/#comment135461>
@Override
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java
<https://reviews.apache.org/r/33096/#comment135466>
why change this logic here? It'd better to not to change it if it isn't a problem.
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
<https://reviews.apache.org/r/33096/#comment135464>
trailing space
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
<https://reviews.apache.org/r/33096/#comment135465>
trailing space
- Sijie Guo
On April 10, 2015, 10:18 p.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33096/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 10:18 p.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-851
> https://issues.apache.org/jira/browse/BOOKKEEPER-851
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Allow to configure a different implementation for the LedgerStorage interface in the Bookie configuration.
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookKeeperServerStats.java 9036182
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 8b40853
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java 1a69692
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java e992d03
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorageFactory.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ReadOnlyEntryLogger.java a2ce9e3
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java d8a87e4
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java cd9f7a0
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java 6c9c4a7
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/TestSyncThread.java d947dff
> bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java 19aab44
>
> Diff: https://reviews.apache.org/r/33096/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>