New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 1316: A bookie with non-writable dirs should be able to start in readonly mode #1319

Closed
wants to merge 6 commits into
base: master
from

Conversation

@sijie
Contributor

sijie commented Apr 6, 2018

Descriptions of the changes in this PR:

Problem

Bookie failed to start when it doesn't have non-writable dirs. Issue is reported at #1316

Solution

  • Introduce a setting minUsageSizeForEntryLogCreation to allow creating entry logs even when there is no writable dirs. This setting is replacing getIsForceGCAllowWhenNoSpace for creating new log, since entry log files are created not only for gc/compaction. It can also happen on startup and also journal replays.

  • Defer creating entry log files until first write happens, when there is no writable ledger dirs.

  • add bunch of tests for EntryLog and Bookie initialization.

Master Issue: #1316
Related Issue: #190


Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

  • Make sure the PR title is formatted like:
    <BP-#>: Description of bookkeeper proposal
    e.g. BP-1: 64 bits ledger is support
  • Attach the master issue link in the description of this PR.
  • Attach the google doc link if the BP is written in Google Doc.

Otherwise:

  • Make sure the PR title is formatted like:
    <Issue #>: Description of pull request
    e.g. Issue 123: Description ...
  • Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.
  • Replace <Issue #> in the title with the actual Issue number.

@sijie sijie self-assigned this Apr 6, 2018

@sijie sijie requested review from jvrao, merlimat and jiazhai Apr 6, 2018

@sijie

This comment has been minimized.

Contributor

sijie commented Apr 6, 2018

@reddycharan - can you review this change? since this is related to your change at #190 (9ddd9e6)

return currentLogId;
}
BufferedLogChannel getCurrentLogChannel() {

This comment has been minimized.

@reddycharan

reddycharan Apr 6, 2018

Contributor

it seems it is being just used for testing purpose, I'm not sure if it is needed. Cann't we rely on just getCurrentLogId? Also, iirc we discussed that entrylogger is exposing more than it is supposed to. For my EntryLogPerLedger, we already compromised on getCurrentLogId (in entrylogperledger case we decided to return some constant value) and I'm not sure if one more such method is ok.

This comment has been minimized.

@sijie

sijie Apr 6, 2018

Contributor

we can't just use currentLogId for testing the behavior of defer creation. defer creation means the currentLogChannel is null.

currentLogId and currentLogChannel are single entry log manager stuffs. they should not be part of the interface, that's why I suggested in our discussion - hiding all the log id stuffs in the implementation. If you follow that suggestion, the currentLogId and currentLogChannel should be just package-protected methods in single entry log manager, and the test EntryLogTest should be changed to test single entry log manager rather than entry logger.

This comment has been minimized.

@reddycharan

reddycharan Apr 6, 2018

Contributor

my basic question is why do we need one more method - getCurrentLogChannel? why cannt we manage your testing needs with just one method here

synchronized long getCurrentLogId() {
    if (logChannel == null) {
      // return some invalid/unassigned id
    } else {
      return logChannel.getLogId();
    }
}

This comment has been minimized.

@reddycharan

reddycharan Apr 6, 2018

Contributor

ideally EntryLogger isn't supposed to expose both currentLogId and currentLogChannel, but we made peace with the fact for currentLogId, but I'm not convenient with one more non-ideal decision.

This comment has been minimized.

@sijie

sijie Apr 6, 2018

Contributor

Current EntryLogger assumes it is single entry log implementation. These methods are package-protected methods (not public method), they are exposed for testing purpose.


If you are thinking from EntryLogManager perspective, these methods should be hidden to single entry log manager implementation. These methods should be only package-protected at single log manager implementation.

If you see inconvenient here, that means your EntryLogManager interface in #1281 is not defining the right interfaces. currentLogId and currentLogChannel should only matter and be testing on single log entry manager. you shouldn't never consider it for per ledger entry manager. If you think and define your EntryLogManager interface in this way, I don't think currentLogId and currentLogChannel is not a problem to you.

This comment has been minimized.

@sijie

sijie Apr 6, 2018

Contributor

@reddycharan

my basic question is why do we need one more method - getCurrentLogChannel? why cannt we manage your testing needs with just one method here

why getCurrentLogChannel is a problem to you? I don't really understand your concern here. are you concerning the method making your interface complicate or what?

as I said if you think this make your interface complicated, that mean your interface doesn't provide the right abstracted methods. The methods I am adding here is only for single entry log implementation, that says currentLogId and currentLogChannel should ONLY belong to single entry log implementation (which current EntryLogger is exactly implemented).

After #1281, these two methods should ONLY moved to single entry log implementation. You shouldn't consider implement these two methods with PerLedgerEntryLogManager, otherwise that means your interface defines wrong methods.

This comment has been minimized.

@reddycharan

reddycharan Apr 6, 2018

Contributor

yeah these aren't public methods, but since there are packaged methods there is a possibility that classes in this package might use it, just like getcurrentlogid is called from SortedLedgerStorage, which I'm afraid of. For these classes, the type of the entrylogmanager should be transparent and at that time entrylogmanager for entrylogperledger should also provide that api.

But still I don't understand why do you need one more variable - currentLogId and one more method - getcurrentlogchannel, when your test cases are just checking if it is null or not null. It is extra burden to keep these vaiables in sync currentlogchannel and currentLogId and maintain one more method - getcurrentlogchannel (which I don't think should be exposed in the first place).

If you still feel strong about it, then let's have just currentLogChannel variable and getCurrentLogChannel method. We can remove currentLogId variable and it's corresponding getter. (for my entrylogmanager for entrylogperledger I would return null for this method).

This comment has been minimized.

@sijie

sijie Apr 6, 2018

Contributor

just like getcurrentlogid is called from SortedLedgerStorage,
for my entrylogmanager for entrylogperledger I would return null for this method

as I pointed out at different places, log id stuffs are implementation specific and they should be hidden to implementation of EntryLogManager. they don't belong to the interface of EntryLogManager. I proposed in the other comment to have a method at EntryLogManager write(SkipLIstFlusher ...), to write a skip list to entry logger, in this way you can hide the single-entrylog-manager implementation into this method without interfering with getCurrentLogId. so SortedLedgerStorage will not call getCurrentLogId any more but the tests can still examine getCurrentLogId through single-log-manager-implementation at white-box testing.

But again this is a discussion should be in #1281 rather than here.

currentLogId and one more method - getcurrentlogchannel, when your test cases are just checking if it is null or not null. It is extra burden to keep these vaiables in sync currentlogchannel and currentLogId and maintain one more method

That's a valid point to ask rather than from EntryLogManager perspective. I will remove getCurrentLogChannel.

This comment has been minimized.

@reddycharan

reddycharan Apr 6, 2018

Contributor

That's a valid point to ask rather than from EntryLogManager perspective. I will remove getCurrentLogChannel.

Peace! thats my concern to begin with, having two variables and two getters when just one should be sufficient.

createNewLog();
if (ledgerDirsManager.hasWritableLedgerDirs()) {
createNewLog();

This comment has been minimized.

@reddycharan

reddycharan Apr 6, 2018

Contributor

Hey @sijie with my change EntryLogManager/EntryLogPerLedger I'm removing initialize method all together. active entrylog (currentlogchannel) would be created on the first addEntry request. So this change wont be required once my change gets in. #1281 . I request you to check my change and hold on this change.

This comment has been minimized.

@sijie

sijie Apr 6, 2018

Contributor

I know that. Your change involves a refactor which take time to get in, and probably we can't take it as 4.7.0 release. This change I am doing here is fixing a bug for 4.7.0 release. I don't think it is needed to couple these two changes together.

This comment has been minimized.

@reddycharan

reddycharan Apr 6, 2018

Contributor

I'm not sure about 4.7.0 release/timelines. But this PR - #1281 shouldn't take more time considering we agreed upon new interface. But anyhow with my change since I'm removing intialize method, the change you made here will be removed as well and since initialize method is removed, Bookie would be able to start with or without any writableLedgerDirs, as this change intend to be.

This comment has been minimized.

@sijie

sijie Apr 6, 2018

Contributor

@reddycharan - I am starting the release 4.7.0 release this week. I don't think #1281 is able to make it to 4.7.0 since it is a bit risky. The change here is much simpler just to defer log creation.

@sijie

This comment has been minimized.

Contributor

sijie commented Apr 6, 2018

@reddycharan I removed the getCurrentLogChannel call.

@@ -176,19 +176,10 @@ public boolean hasWritableLedgerDirs() {
return writableLedgerDirectories;
}
// If Force GC is not allowed under no space
if (!forceGCAllowWhenNoSpace) {

This comment has been minimized.

@reddycharan

reddycharan Apr 6, 2018

Contributor

any reason for why this check is removed?

This comment has been minimized.

@sijie

sijie Apr 6, 2018

Contributor
  1. forceGCAllowWhenNoSpace is controlling the gc behavior, it controls whether to gc/compaction
  2. this setting is replaced by minUsableSizeForEntryLogCreation. because not only gc/compaction will create the entry log files on readonly, during journal replays it would still need create entry log files. This allows we disableForceGC when no space but we can still allow bookie start up in readonly mode to replay journals if there is enough space.
@reddycharan

This comment has been minimized.

Contributor

reddycharan commented Apr 6, 2018

LGTM.

But with my change EntryLogManager/EntryLogPerLedger I'm removing initialize method all together. active entrylog (currentlogchannel) would be created on the first addEntry request.

@sijie sijie added this to the 4.7.0 milestone Apr 6, 2018

@sijie sijie added the release/4.7.0 label Apr 6, 2018

@sijie sijie closed this in ea1a75c Apr 6, 2018

@sijie sijie deleted the sijie:allow_bookie_to_start_with_no_writable_bookies branch Jul 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment