You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/12/03 20:41:58 UTC

[kudu-CR] log: remove SegmentAllocator functors

Hello Kudu Jenkins, Andrew Wong, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14819

to look at the new patch set (#2).

Change subject: log: remove SegmentAllocator functors
......................................................................

log: remove SegmentAllocator functors

When we roll the log, we close the active segment before switching to the
next preallocated segment. What happens when we close the active segment?
1. We sync it.
2. We write out the (now complete) footer.
3. We close the segment file. Under the hood, this will truncate any unused
   preallocated space out of the file and sync it one last time.
4. We open the closed segment file for reading. Note that this is the second
   time we've opened it; we also opened it once when we last rolled the log.
5. We create a new in-memory segment reader. To avoid needless IO, we
   initialize it using the in-memory header and footer, but we do end up
   reloading the segment's file size from disk.
6. We replace the last segment reader in the log with the reader constructed
   in step #5. Side effect: the original segment reader is now closed.

My original intent with this patch was to replace steps 4-6 with a simpler
"inject the footer into the existing in-memory segment reader". This proved
to be impossible without adding a new synchronization primitive to protect
the footer in the segment reader, which is something I wanted to avoid. And
the win would have been minimal: with proper file cache usage, step 4 is
likely to be a cache hit.

So now this patch just cleans up SegmentAllocator by removing its functors,
which I found difficult to follow. Instead, Log now manipulates LogReader
directly, using information passed back from SegmentAllocator functions.

Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
---
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/log-test-base.h
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/integration-tests/timestamp_advancement-itest.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tserver/tablet_copy-test-base.h
12 files changed, 188 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/14819/2
-- 
To view, visit http://gerrit.cloudera.org:8080/14819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)