SortedLedgerStorage supports multiple dirs #3493
Conversation
StevenLuMT
force-pushed
the
master_SortedLedgerStorageMultiDirs
branch
from
d736bf8 to
d201d74
Compare
StevenLuMT
force-pushed
the
master_SortedLedgerStorageMultiDirs
branch
from
0848afb to
ecd4eb2
Compare
| interleavedLedgerStorage.checkpointer.startCheckpoint(cp); | ||
| for (InterleavedLedgerStorage s : interleavedLedgerStorageList) { | ||
| try { | ||
| LOG.info("Started flushing mem table."); |
There was a problem hiding this comment.
This line will be logged multiple times. Can we log something about 's'?
There was a problem hiding this comment.
finished:
print log's dir
| s.checkpointer.startCheckpoint(cp); | ||
| } | ||
| } catch (Exception e) { | ||
| s.getStateManager().transitionToReadOnlyMode(); |
There was a problem hiding this comment.
finished:
print log's dir
|
|
||
| public DefaultEntryLogger getEntryLogger() { | ||
| return interleavedLedgerStorage.getEntryLogger(); | ||
| public List<DefaultEntryLogger> getEntryLogger() { |
| } | ||
|
|
||
| @Override | ||
| public void forceGC() { | ||
| interleavedLedgerStorage.forceGC(); | ||
| interleavedLedgerStorageList.forEach(s -> s.forceGC()); |
There was a problem hiding this comment.
If one of the instances throws an error we won't process the others.
I suggest to wrap the error and continue processing.
Then, if one error occurred we throw it in the end.
The same here and in other similar methods
There was a problem hiding this comment.
finished:
exception handling optimization
| } | ||
|
|
||
| @Override | ||
| public LedgerStorage getUnderlyingLedgerStorage() { | ||
| return interleavedLedgerStorage; | ||
| return interleavedLedgerStorageList.get(0); |
There was a problem hiding this comment.
I am not sure that this is correct.
Is this method only for tests?
There was a problem hiding this comment.
finished:
this is mistake,I have updated
| } | ||
|
|
||
| @VisibleForTesting | ||
| protected SortedLedgerStorage(InterleavedLedgerStorage ils) { | ||
| interleavedLedgerStorage = ils; | ||
| protected SortedLedgerStorage(String interleavedLedgerStorageClazz) { |
There was a problem hiding this comment.
Why do we pass a class name here?
There was a problem hiding this comment.
this is for InterleavedLedgerStorage's child class: SlowInterleavedLedgerStorage
we can init SortedLedgerStorage with overide the diff child InterleavedLedgerStorage
please provide more details - what's the bug / how it affects BK. |
| LOG.info("ledgerStorage({}) started flushing mem table.", | ||
| getEntryLogDirPath(s.getEntryLogger())); | ||
| s.getEntryLogger().prepareEntryMemTableFlush(); | ||
| memTable.flush(SortedLedgerStorage.this); |
There was a problem hiding this comment.
I think there is no need to do memetable.flush for each interleaved storage, Sorted storage should fan flush out.
| s.getEntryLogger().prepareEntryMemTableFlush(); | ||
| memTable.flush(SortedLedgerStorage.this); | ||
|
|
||
| if (s.getEntryLogger().commitEntryMemTableFlush()) { |
There was a problem hiding this comment.
it is possible that only some interleaved storages / entry loggers actually got data (e.g. memetaable had data for one ledger only). What's the side-effect of the prepare/commit flush without any actual data? Do we have tests for this?
| s.checkpointer.startCheckpoint(cp); | ||
| } | ||
| } catch (Exception e) { | ||
| s.getStateManager().transitionToReadOnlyMode(); |
There was a problem hiding this comment.
Now we create interleaved storages (ILS) per directory (ledgerDirsManager.getAllLedgerDirs().size())
Entry log manager (EntryLogManagerBase, getCurrentLogForLedgerForAddEntry/createNewLog/selectDirForNextEntryLog) still rotates entry logs across all available directories (the only reason why transition to read only on one ILS failing write is acceptable.) Also there is entry log per ledger.
It looks like it implicitly introduces parallelism (N ILSs for N dirs -> will write to N entry loggers, possibly at the same dir) which might not be ok for rotational disks
i do not have clear picture of what specifically you are trying to achieve so this per my best understanding.
|
@dlg99 thank you for your comments, I will sort it out and reply to you later |
|
Temporarily close first, and reopen after improvement |
Descriptions of the changes in this PR:
Motivation
planning for multiple dirs: mail talking
stage 1 :
the condition that the bug triggers:
1) conf.getLedgerDirs are multiple dirs
2) ledgerStorageClass=org.apache.bookkeeper.bookie.SortedLedgerStorage
Changes