[bookie] Fix sorted ledger storage rotating entry log files too frequent #1807

sijie · 2018-11-12T19:46:10Z

Descriptions of the changes in this PR:

Motivation

A strong behavior was observed when using sorted ledger storage with single entry log manager on production:

"the entry log files are rotated very frequently and small entry log files are produced".

The problem was introduced due to #1410.

At current entry logger, when a new entry log file is created, EntryLogger will notify its listeners
that a new entry log file is rotated via onRotateEntryLog.

Before the change in #1410, SortedLedgerStorage inherits from InterleavedLedgerStorage.
So when a new entry log file is rotated, SortedLedgerStorage is notified.

However after the change in #1410, SortedLedgerStorage doesn't inherits InterleavedLedgerStorage anymore.
Instead, the relationship is changed to composition. SortedLedgerStorage is composed using an interleaved ledger
storage. So the entrylog listener contract was broken. SortedLedgerStorage will not receive any onRotateEntryLog
notification any more.

Changes

When SortedLedgerStorage initializes, it passes its own entry log listener down to the interleaved ledger storage.
So entry logger can notify the right person for entry log rotations.

Tests

Existing tests should cover most of the case. Looking for how to add new test cases.

In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
checks for pull requests. A pull request can only be merged when it passes precommit checks. However running all
the precommit checks can take a long time, some trivial changes don't need to run all the precommit checks. You
can check following list to skip the tests that don't need to run for your pull request. Leave them unchecked if
you are not sure, committers will help you:

[skip bookkeeper-server bookie tests]: skip testing org.apache.bookkeeper.bookie in bookkeeper-server module.

[skip bookkeeper-server client tests]: skip testing org.apache.bookkeeper.client in bookkeeper-server module.

[skip bookkeeper-server replication tests]: skip testing org.apache.bookkeeper.replication in bookkeeper-server module.

[skip bookkeeper-server tls tests]: skip testing org.apache.bookkeeper.tls in bookkeeper-server module.

[skip bookkeeper-server remaining tests]: skip testing all other tests in bookkeeper-server module.

[skip integration tests]: skip docker based integration tests. if you make java code changes, you shouldn't skip integration tests.

[skip build java8]: skip build on java8. ONLY skip this when ONLY changing files under documentation under site.

[skip build java9]: skip build on java9. ONLY skip this when ONLY changing files under documentation under site.

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.

*Motivation* A strong behavior was observed when using sorted ledger storage with single entry log manager on production: "the entry log files are rotated very frequently and small entry log files are produced". The problem was introduced due to apache#1410. At current entry logger, when a new entry log file is created, EntryLogger will notify its listeners that a new entry log file is rotated via [`onRotateEntryLog`](https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogManagerBase.java#L152). Before the change in apache#1410, `SortedLedgerStorage` inherits from `InterleavedLedgerStorage`. So when a new entry log file is rotated, `SortedLedgerStorage` is notified. However after the change in apache#1410, `SortedLedgerStorage` doesn't inherits `InterleavedLedgerStorage` anymore. Instead, the relationship is changed to composition. `SortedLedgerStorage` is composed using an interleaved ledger storage. So the entrylog listener contract was broken. `SortedLedgerStorage` will not receive any `onRotateEntryLog` notification any more. *Changes* When `SortedLedgerStorage` initializes, it passes its own entry log listener down to the interleaved ledger storage. So entry logger can notify the right person for entry log rotations. *Tests* Existing tests should cover most of the case. Looking for how to add new test cases.

sijie · 2018-11-12T19:47:38Z

(not sure if that's the best fix or not. any suggestions are welcome)

reddycharan · 2018-11-12T19:51:37Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java

+            statsLogger);
+    }
+
+    void initializeWithEntryLogListener(ServerConfiguration conf,


instead of adding a new initialize method, you should consider changing the signature of the existing initialize method, which takes EntryLogListener argument. In ILS initialize method it will check if it receives the listener, if it is not null use it otherwise use ‘this’.

Actually I think currently entry log listener is only visible to interleaved and sorte ledger storages. but it is not visible to db ledger storage. so I think it is probably better to use my approach here, since the behavior is only applied to interleaved and sorted ledger storage.

we can improve it later if this listener is going to be applied to all ledger storage implementation.

eolivelli

@reddycharan I am not sure your proposal is clearer. Because it will add an implicit default.

To me we can go with this change, but I have no strong feeling

reddycharan · 2018-11-12T20:48:52Z

@sijie in this change can you add log files in the stackTrace of createNewLog call hierarchy. As we observed in the log file, it was not evident what triggered the creation of new entrylog files. Since createNewLog is infrequent thing, it should be ok to add logs as much as we want.

sijie · 2018-11-13T08:49:47Z

@reddycharan I added a logging message with a reason field. let me know if this approach works for you.

reddycharan · 2018-11-13T16:35:37Z

...r-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogManagerForEntryLogPerLedger.java

-                createNewLog(ledgerId);
+                createNewLog(ledgerId,
+                    ": diskFull = " + diskFull + ", allDisksFull = " + allDisksFull
+                        + ", reachEntryLogLimit = " + reachEntryLogLimit);


we can get to this block even if logChannel is null. so add that aswell to the reason string.

add logChannel in the reason string.

reddycharan

LGTM other than that minor log statement issue - https://github.com/apache/bookkeeper/pull/1807/files#r233123238

sijie · 2018-11-14T23:19:51Z

run integration tests

sijie · 2018-11-16T22:10:03Z

IGNORE IT CI

Descriptions of the changes in this PR: *Motivation* A strong behavior was observed when using sorted ledger storage with single entry log manager on production: "the entry log files are rotated very frequently and small entry log files are produced". The problem was introduced due to #1410. At current entry logger, when a new entry log file is created, EntryLogger will notify its listeners that a new entry log file is rotated via [`onRotateEntryLog`](https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogManagerBase.java#L152). Before the change in #1410, `SortedLedgerStorage` inherits from `InterleavedLedgerStorage`. So when a new entry log file is rotated, `SortedLedgerStorage` is notified. However after the change in #1410, `SortedLedgerStorage` doesn't inherits `InterleavedLedgerStorage` anymore. Instead, the relationship is changed to composition. `SortedLedgerStorage` is composed using an interleaved ledger storage. So the entrylog listener contract was broken. `SortedLedgerStorage` will not receive any `onRotateEntryLog` notification any more. *Changes* When `SortedLedgerStorage` initializes, it passes its own entry log listener down to the interleaved ledger storage. So entry logger can notify the right person for entry log rotations. *Tests* Existing tests should cover most of the case. Looking for how to add new test cases. Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Charan Reddy Guttapalem <reddycharan18@gmail.com>, Andrey Yegorov <None> This closes #1807 from sijie/fix_rotation_behavior (cherry picked from commit deebe6d) Signed-off-by: Sijie Guo <sijie@apache.org>

sijie requested review from dlg99 and reddycharan November 12, 2018 19:46

reddycharan reviewed Nov 12, 2018

View reviewed changes

eolivelli approved these changes Nov 12, 2018

View reviewed changes

dlg99 approved these changes Nov 12, 2018

View reviewed changes

sijie added 2 commits November 13, 2018 00:20

Merge remote-tracking branch 'apache/master' into fix_rotation_behavior

e0f8ae1

Add "reason" on creating new log

5ecf449

reddycharan reviewed Nov 13, 2018

View reviewed changes

reddycharan approved these changes Nov 13, 2018

View reviewed changes

Addressed review comments

a64ff8b

reddycharan approved these changes Nov 14, 2018

View reviewed changes

sijie added this to the 4.9.0 milestone Nov 16, 2018

sijie added area/bookie release/4.9.0 type/bug labels Nov 16, 2018

sijie self-assigned this Nov 16, 2018

sijie merged commit deebe6d into apache:master Nov 16, 2018

sijie added the release/4.8.2 label Nov 16, 2018

sijie deleted the fix_rotation_behavior branch November 16, 2018 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bookie] Fix sorted ledger storage rotating entry log files too frequent #1807

[bookie] Fix sorted ledger storage rotating entry log files too frequent #1807

sijie commented Nov 12, 2018

sijie commented Nov 12, 2018

reddycharan Nov 12, 2018

sijie Nov 13, 2018

eolivelli left a comment

reddycharan commented Nov 12, 2018

sijie commented Nov 13, 2018

reddycharan Nov 13, 2018

sijie Nov 14, 2018

reddycharan left a comment

sijie commented Nov 14, 2018

sijie commented Nov 16, 2018

[bookie] Fix sorted ledger storage rotating entry log files too frequent #1807

[bookie] Fix sorted ledger storage rotating entry log files too frequent #1807

Conversation

sijie commented Nov 12, 2018

sijie commented Nov 12, 2018

reddycharan Nov 12, 2018

Choose a reason for hiding this comment

sijie Nov 13, 2018

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

reddycharan commented Nov 12, 2018

sijie commented Nov 13, 2018

reddycharan Nov 13, 2018

Choose a reason for hiding this comment

sijie Nov 14, 2018

Choose a reason for hiding this comment

reddycharan left a comment

Choose a reason for hiding this comment

sijie commented Nov 14, 2018

sijie commented Nov 16, 2018