Skip to content
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

Cleanup logfileHeader in DefaultEntryLogger #4146

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

AnonHxy
Copy link
Contributor

@AnonHxy AnonHxy commented Dec 6, 2023

Descriptions of the changes in this PR:

Motivation

Cleanup logfileHeader in DefaultEntryLogger. There is no place to invoke DefaultEntryLogger#logfileHeader to write, and the logfileHeader has moved to EntryLoggerAllocator:

final ByteBuf logfileHeader = Unpooled.buffer(DefaultEntryLogger.LOGFILE_HEADER_SIZE);

// Initialize the entry log header buffer. This cannot be a static object
// since in our unit tests, we run multiple Bookies and thus EntryLoggers
// within the same JVM. All of these Bookie instances access this header
// so there can be race conditions when entry logs are rolled over and
// this header buffer is cleared before writing it into the new logChannel.
logfileHeader.writeBytes("BKLO".getBytes(UTF_8));
logfileHeader.writeInt(DefaultEntryLogger.HEADER_CURRENT_VERSION);
logfileHeader.writerIndex(DefaultEntryLogger.LOGFILE_HEADER_SIZE);

logfileHeader.readerIndex(0);
logChannel.write(logfileHeader);

Changes

Cleanup logfileHeader in DefaultEntryLogger

@hangc0276
Copy link
Contributor

@AnonHxy Please rebase the master, thanks.

@AnonHxy AnonHxy closed this Dec 7, 2023
@AnonHxy AnonHxy force-pushed the cleanup_entrylog_header_write branch from 924ab55 to be1749c Compare December 7, 2023 14:41
@AnonHxy AnonHxy reopened this Dec 7, 2023
@AnonHxy AnonHxy closed this Dec 8, 2023
@AnonHxy AnonHxy reopened this Dec 8, 2023
@AnonHxy
Copy link
Contributor Author

AnonHxy commented Dec 8, 2023

@AnonHxy Please rebase the master, thanks.

Done @hangc0276 @zymap PTAL

@hangc0276 hangc0276 requested review from eolivelli and zymap December 11, 2023 06:19
@hangc0276 hangc0276 added this to the 4.17.0 milestone Dec 11, 2023
@merlimat merlimat merged commit c40f756 into apache:master Jan 9, 2024
32 checks passed
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants