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 01:20:12 UTC

[kudu-CR] log: don't reopen just-closed active segments 

Hello Andrew Wong,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: log: don't reopen just-closed active segments
......................................................................

log: don't reopen just-closed active segments

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.

The effort to reduce IO is laudable, but we can do better by not reopening
the segment file at all: it is sufficient to inject the now complete footer
into the existing reader, and to recalculate the file size (although this is
probably only needed for tests, it was simpler to always do it).

Along the way, I cleaned up SegmentAllocator by removing its functors, which
I found difficult to follow. Instead, Log now maniupates 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, 181 insertions(+), 211 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/14819/1
-- 
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: newchange
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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)

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14819 )

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


Patch Set 4: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14819/3/src/kudu/consensus/log_reader.h
File src/kudu/consensus/log_reader.h:

http://gerrit.cloudera.org:8080/#/c/14819/3/src/kudu/consensus/log_reader.h@58
PS3, Line 58: each of these patterns
this pattern

?



-- 
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: comment
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Dec 2019 18:10:14 +0000
Gerrit-HasComments: Yes

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, 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 (#6).

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, 211 insertions(+), 190 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/14819/6
-- 
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: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14819 )

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
Reviewed-on: http://gerrit.cloudera.org:8080/14819
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Kudu Jenkins
---
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, 211 insertions(+), 190 deletions(-)

Approvals:
  Andrew Wong: Looks good to me, approved
  Kudu Jenkins: Verified

-- 
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: merged
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14819 )

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


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14819/3/src/kudu/consensus/log_reader.h
File src/kudu/consensus/log_reader.h:

http://gerrit.cloudera.org:8080/#/c/14819/3/src/kudu/consensus/log_reader.h@58
PS3, Line 58: each of these patterns
> this pattern
What I'm trying to say is that UpdateLastSegmentOffset is an example of "mutating a segment in a thread-safe manner", and ReplaceLastSegment is an example of "make a copy of the segment, modify what you want to modify, then replace the segment atomically". These are two different access patterns, hence the use of 'patterns' in the plural.

Can you help me come up with a clearer way to express this?



-- 
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: comment
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Dec 2019 18:31:47 +0000
Gerrit-HasComments: Yes

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14819 )

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


Patch Set 2: Verified+1

Overriding Jenkins, unrelated known test flake.


-- 
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: comment
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 03 Dec 2019 21:16:02 +0000
Gerrit-HasComments: No

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14819 )

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


Patch Set 7: Code-Review+2

Thanks for the cleanup! Looks way better now.


-- 
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: comment
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 04 Dec 2019 22:06:06 +0000
Gerrit-HasComments: No

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14819 )

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


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14819/3/src/kudu/consensus/log_reader.h
File src/kudu/consensus/log_reader.h:

http://gerrit.cloudera.org:8080/#/c/14819/3/src/kudu/consensus/log_reader.h@58
PS3, Line 58: LastSegment are exampl
> No, I misread this.
Done



-- 
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: comment
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Dec 2019 19:24:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14819 )

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


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14819/3/src/kudu/consensus/log_reader.h
File src/kudu/consensus/log_reader.h:

http://gerrit.cloudera.org:8080/#/c/14819/3/src/kudu/consensus/log_reader.h@58
PS3, Line 58: each of these patterns
> add "respectively"
No, I misread this.

Once initialized, care should be taken to avoid mutating the segments in a non-thread-safe manner. E.g. segment mutation should be made thread-safe with locking primitives, or done via a copy-then-replace access pattern. UpdateLastSegmentOffset and ReplaceLastSegment are examples of each these patterns respectively.



-- 
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: comment
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Dec 2019 19:16:50 +0000
Gerrit-HasComments: Yes

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14819 )

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


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14819/3/src/kudu/consensus/log_reader.h
File src/kudu/consensus/log_reader.h:

http://gerrit.cloudera.org:8080/#/c/14819/3/src/kudu/consensus/log_reader.h@58
PS3, Line 58: each of these patterns
> What I'm trying to say is that UpdateLastSegmentOffset is an example of "mu
add "respectively"



-- 
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: comment
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Dec 2019 19:12:16 +0000
Gerrit-HasComments: Yes

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/14819 )

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


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
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: deleteReviewer
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14819 )

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


Patch Set 4: Code-Review+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: comment
Gerrit-Change-Id: Id2f5dd47e3be930288dcfeb1f77409fda45daac4
Gerrit-Change-Number: 14819
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Dec 2019 18:44:06 +0000
Gerrit-HasComments: No

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, 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 (#3).

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, 198 insertions(+), 179 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/14819/3
-- 
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: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>

[kudu-CR] log: remove SegmentAllocator functors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, 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 (#5).

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, 199 insertions(+), 179 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/14819/5
-- 
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: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)