BP-47 (task3): Abstract interface for entrylogger by hangc0276 · Pull Request #3197 · apache/bookkeeper

hangc0276 · 2022-04-10T10:59:57Z

Motivation

Define the interface and contract for entrylogger. This is mostly taking the entrylogger methods used by other components and prettying them up a bit.

Notable Changes

Abstract EntryLogger and CompactionEntryLog interface. Provide common interface for read/write entries and entry log compaction.
Rename EntryLogger class to DefaultEntryLogger to represent default entryLogger implementation
Put the EntryLogScanner interface out of EntryLogger class
shutdown has been renamed to close.
the compaction entrylog methods have been put behind the CompactionEntryLog interface.

Sub task of #2932

Others

The commit is made by @ivankelly . This is the sub task of #2932 , which pushed out by @mauricebarnum. However, this PR contains too many changes and the number of code lines reaches 8K+, and it is hard to review. According to your suggestion #2932 (comment), and after communicate with @mauricebarnum, we are planing to divide the PR into 6 PRs, Please refer to #2943 (comment).

However, @mauricebarnum doesn't have enough time to deal with those issues, and we desperately need this feature. After communicated with @mauricebarnum and @merlimat , I help to work on divide the PRs and push them out. We are hoping to contain this feature in BookKeeper 4.16.0

hangc0276 · 2022-04-18T09:07:43Z

@merlimat @eolivelli @dlg99 @mauricebarnum Please help take a look, Thanks a lot.

eolivelli · 2022-04-18T12:53:00Z

It looks like the first commit was made by @ivankelly ?

We should keep this reference and explicitly state this in the description of the PR

hangc0276 · 2022-04-18T16:23:47Z

It looks like the first commit was made by @ivankelly ?

We should keep this reference and explicitly state this in the description of the PR

@eolivelli Yes, The commit made by @ivankelly . This is the sub task of #2932 , which pushed out by @mauricebarnum. However, this PR contains too many changes and the number of code lines reaches 8K+, and it is hard to review. According to your suggestion #2932 (comment), and after communicate with @mauricebarnum, we are planing to divide the PR into 6 PRs, Please refer to #2943 (comment).

However, @mauricebarnum doesn't have enough time to deal with those issues, and we desperately need this feature. After communicated with @mauricebarnum and @merlimat , I help to work on divide the PRs and push them out. We are hoping to contain this feature in BookKeeper 4.16.0

Define the interface and contract for entrylogger. This is mostly taking the entrylogger methods used by other components and prettying them up a bit. Notable changes are: - internalReadEntry is now readEntry. There is no 'validate' flag. Instead there are two overloads for the method, and validation only runs if ledgerId and entryId are passed in. - shutdown has been renamed to close. - the compaction entrylog methods have been put behind an interface. As it was they were leaking implementation details. Ultimitely compaction itself should be hidden behind the entrylogger, but that's a larger refactor. (cherry picked from commit c927f4b)

eolivelli

LGTM

@merlimat @dlg99 could you please take a look?

dlg99

Overall LGTM.

Couple of points to address:

UNASSIGNED_LEDGERID should be kept in the EntryLogger interface to be available/the same in all implementations
fields and method params should use interface instead of concrete implementation

dlg99 · 2022-04-27T20:47:15Z

    static final long UNASSIGNED_LEDGERID = -1L;
    // log file suffix
    static final String LOG_FILE_SUFFIX = ".log";


I think these should be kept in the EntryLogger interface because these are not implementation-specific (UNASSIGNED_LEDGERID - definitely, LOG_FILE_SUFFIX - discussable)

Co-authored-by: Andrey Yegorov <8622884+dlg99@users.noreply.github.com>

hangc0276 · 2022-04-28T02:11:21Z

fields and method params should use interface instead of concrete implementation

@dlg99 Thanks you for your patient review.

fields and method params should use interface instead of concrete implementation

For current purposes, we will support dbLedgerStorage to use directIOEntryLogger first, and for other kind ledgerStorage implementation, we will consider to support later. For other kinds of ledgerStorage support, we should do more work on the EntryLogger interface abstraction in the future. So my suggestion is that currently we use DefaultEntryLogger for the fields and method params in other kinds of ledgerStorage.

hangc0276 · 2022-04-28T02:47:39Z

rerun failure checks

hangc0276 · 2022-04-28T05:07:31Z

rerun failure checks

hangc0276 · 2022-04-28T12:40:10Z

rerun failure checks

hangc0276 · 2022-04-28T14:12:29Z

rerun failure checks

* Interface for entrylogger Define the interface and contract for entrylogger. This is mostly taking the entrylogger methods used by other components and prettying them up a bit. Notable changes are: - internalReadEntry is now readEntry. There is no 'validate' flag. Instead there are two overloads for the method, and validation only runs if ledgerId and entryId are passed in. - shutdown has been renamed to close. - the compaction entrylog methods have been put behind an interface. As it was they were leaking implementation details. Ultimitely compaction itself should be hidden behind the entrylogger, but that's a larger refactor. (cherry picked from commit c927f4b) * abstract interface for entrylogger * format code * format code * address comments * Apply suggestions from code review Co-authored-by: Andrey Yegorov <8622884+dlg99@users.noreply.github.com> * address comments * format code Co-authored-by: Ivan Kelly <ikelly@splunk.com> Co-authored-by: Andrey Yegorov <8622884+dlg99@users.noreply.github.com>

eolivelli requested changes Apr 10, 2022

View reviewed changes

Comment thread bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java Outdated

Comment thread bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/EntryLoggerIface.java Outdated

hangc0276 force-pushed the chenhang/define_interface_for_entrylogger branch from dee84ae to 777918b Compare April 18, 2022 06:15

hangc0276 changed the title ~~BP-47 (task3): Define interface for entrylogger~~ BP-47 (task3): Abstract interface for entrylogger Apr 18, 2022

hangc0276 requested a review from eolivelli April 18, 2022 09:06

eolivelli requested changes Apr 18, 2022

View reviewed changes

Comment thread bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java

Comment thread bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/CompactionEntryLog.java

zymap assigned hangc0276 and ivankelly Apr 21, 2022

Ivan Kelly and others added 5 commits April 27, 2022 12:20

abstract interface for entrylogger

5bfbaa1

format code

adca829

format code

8c5dee0

address comments

f0e8b3a

hangc0276 force-pushed the chenhang/define_interface_for_entrylogger branch from 26926b5 to f0e8b3a Compare April 27, 2022 04:25

hangc0276 requested a review from eolivelli April 27, 2022 04:30

eolivelli approved these changes Apr 27, 2022

View reviewed changes

dlg99 approved these changes Apr 27, 2022

View reviewed changes

hangc0276 and others added 2 commits April 28, 2022 09:26

Apply suggestions from code review

3407fcf

Co-authored-by: Andrey Yegorov <8622884+dlg99@users.noreply.github.com>

address comments

9a1e997

format code

da2bac4

eolivelli merged commit 6f85e6d into apache:master Apr 29, 2022

Conversation

hangc0276 commented Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Notable Changes

Others

Uh oh!

Uh oh!

Uh oh!

hangc0276 commented Apr 18, 2022

Uh oh!

Uh oh!

Uh oh!

eolivelli commented Apr 18, 2022

Uh oh!

hangc0276 commented Apr 18, 2022

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

dlg99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dlg99 Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

hangc0276 commented Apr 28, 2022

Uh oh!

hangc0276 commented Apr 28, 2022

Uh oh!

hangc0276 commented Apr 28, 2022

Uh oh!

hangc0276 commented Apr 28, 2022

Uh oh!

hangc0276 commented Apr 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hangc0276 commented Apr 10, 2022 •

edited

Loading