You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2017/09/03 08:45:05 UTC

[kudu-CR] KUDU-2131: switch to FILO log container retrieval 

Hao Hao has uploaded a new change for review.

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

Change subject: KUDU-2131: switch to FILO log container retrieval
......................................................................

KUDU-2131: switch to FILO log container retrieval

Currently, LBM selects available log container from available container
queue using FIFO, given the intention to spread the load across all
available containers. However, this may cause inefficiency especially
in the case for tablet copy if there is a long available containers queue.
One failed test case is copying tablet of size 26.81GB on a tablet server with
14 disks and 19199 containers(only 97 of them are full). In this case, there
are too many containers involved, so that DownloadBlocks took >3m (default
idle timeout) to commit the new blocks such that the session is terminated
before DownloadWALs.

This patch switches to FILO for log container selection, so that finalized
blocks will exhaust one container at a time. Thus, when batching disk
synchronization for a groups of created blocks, the amount of fsync()
will be significant lower for tablet servers with plenty available log
containers.

It also marks '--tablet_copy_idle_timeout_ms', the flag that controls the
amount of time without activity before a tablet copy session expires, to be
a user facing flag. And rename it to '--tablet_copy_idle_timeout_sec'.

Change-Id: I6eb0bc99422c293417b64e0b6e86c87a3d55e687
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_service.cc
5 files changed, 52 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/7952/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6eb0bc99422c293417b64e0b6e86c87a3d55e687
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>

[kudu-CR] KUDU-2131: switch to LIFO log container retrieval

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-2131: switch to LIFO log container retrieval
......................................................................


Patch Set 2: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb0bc99422c293417b64e0b6e86c87a3d55e687
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-2131: switch to LIFO log container retrieval

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change.

Change subject: KUDU-2131: switch to LIFO log container retrieval
......................................................................


Patch Set 2:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/7952/1//COMMIT_MSG
Commit Message:

PS1, Line 7: LIFO
> Nit: not that FILO isn't descriptive, but I think LIFO is more conventional
Done


PS1, Line 12: of 
> of
Done


PS1, Line 14: 14 disks and 19
> containers (only
Done


PS1, Line 15: t
            : DownloadBloc
> You should clarify that this is the default idle timeout for a tablet copy 
Done


PS1, Line 21: e. Thu
> number of fsync() operations
Done


PS1, Line 22: ted bl
> plenty of
Done


PS1, Line 27: ut act
> renames
Done


PS1, Line 26: 
            : t
> as a
Done


http://gerrit.cloudera.org:8080/#/c/7952/1/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:

Line 1380: // Test available log container selection is LIFO.
> LIFO here too.
Done


Line 1397:   // The first available container in the queue will be reused every time.
> "The first available container in the queue will be reused every time."
Done


PS1, Line 1405: finalizi
> finalizing
Done


http://gerrit.cloudera.org:8080/#/c/7952/1/src/kudu/tserver/tablet_copy_service-test.cc
File src/kudu/tserver/tablet_copy_service-test.cc:

PS1, Line 492: FLAGS_tablet_copy_idle_timeout_sec = 0; // Ex
> This is going to perform integer division and result in 0, so we should jus
Done


http://gerrit.cloudera.org:8080/#/c/7952/1/src/kudu/tserver/tablet_copy_service.cc
File src/kudu/tserver/tablet_copy_service.cc:

PS1, Line 101: FromSeconds(FLAG
> Should be FromSeconds.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb0bc99422c293417b64e0b6e86c87a3d55e687
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2131: switch to LIFO log container retrieval

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

Change subject: KUDU-2131: switch to LIFO log container retrieval
......................................................................


KUDU-2131: switch to LIFO log container retrieval

Currently, LBM selects available log container from available container
queue using FIFO, given the intention to spread the load across all
available containers. However, this may cause inefficiency especially
in the case of tablet copy if there is a long available containers
queue. One failed test case is copying tablet of size 26.81GB on a
tablet server with 14 disks and 19199 containers (only 97 of them are
full). In this case, there are too many containers involved, so that
DownloadBlocks took longer that 3m, the default idle timeout for a
tablet copy session, to commit the new blocks such that the session is
terminated before DownloadWALs.

This patch switches to LIFO for log container selection, so that
finalized blocks will exhaust one container at a time. Thus, when
batching disk synchronization for a groups of created blocks, the
number of fsync() operations will be significant lower for tablet
servers with plenty of available log containers.

It also marks '--tablet_copy_idle_timeout_ms', the flag that controls
the amount of time without activity before a tablet copy session
expires, as a user facing flag. And renames it to
'--tablet_copy_idle_timeout_sec'.

Change-Id: I6eb0bc99422c293417b64e0b6e86c87a3d55e687
Reviewed-on: http://gerrit.cloudera.org:8080/7952
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_service.cc
5 files changed, 52 insertions(+), 9 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/7952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I6eb0bc99422c293417b64e0b6e86c87a3d55e687
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] KUDU-2131: switch to FILO log container retrieval

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-2131: switch to FILO log container retrieval
......................................................................


Patch Set 1:

(1 comment)

Thanks for changing the default tablet copy idle timeout.

Adar is more familiar with the log block manager container selection code so I'll let him comment on that part.

http://gerrit.cloudera.org:8080/#/c/7952/1/src/kudu/tserver/tablet_copy_service-test.cc
File src/kudu/tserver/tablet_copy_service-test.cc:

PS1, Line 492: FLAGS_tablet_copy_idle_timeout_sec = 1 / 1000
This is going to perform integer division and result in 0, so we should just set it to 0 and update the comment.


-- 
To view, visit http://gerrit.cloudera.org:8080/7952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb0bc99422c293417b64e0b6e86c87a3d55e687
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2131: switch to LIFO log container retrieval

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2131: switch to LIFO log container retrieval
......................................................................

KUDU-2131: switch to LIFO log container retrieval

Currently, LBM selects available log container from available container
queue using FIFO, given the intention to spread the load across all
available containers. However, this may cause inefficiency especially
in the case of tablet copy if there is a long available containers
queue. One failed test case is copying tablet of size 26.81GB on a
tablet server with 14 disks and 19199 containers (only 97 of them are
full). In this case, there are too many containers involved, so that
DownloadBlocks took longer that 3m, the default idle timeout for a
tablet copy session, to commit the new blocks such that the session is
terminated before DownloadWALs.

This patch switches to LIFO for log container selection, so that
finalized blocks will exhaust one container at a time. Thus, when
batching disk synchronization for a groups of created blocks, the
number of fsync() operations will be significant lower for tablet
servers with plenty of available log containers.

It also marks '--tablet_copy_idle_timeout_ms', the flag that controls
the amount of time without activity before a tablet copy session
expires, as a user facing flag. And renames it to
'--tablet_copy_idle_timeout_sec'.

Change-Id: I6eb0bc99422c293417b64e0b6e86c87a3d55e687
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_copy_service.cc
5 files changed, 52 insertions(+), 9 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6eb0bc99422c293417b64e0b6e86c87a3d55e687
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>

[kudu-CR] KUDU-2131: switch to FILO log container retrieval

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-2131: switch to FILO log container retrieval
......................................................................


Patch Set 1:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/7952/1//COMMIT_MSG
Commit Message:

PS1, Line 7: FILO
Nit: not that FILO isn't descriptive, but I think LIFO is more conventional to describe stack-like behavior.


PS1, Line 12: for
of


PS1, Line 14: containers(only
containers (only


PS1, Line 15: default
            : idle timeout
You should clarify that this is the default idle timeout for a tablet copy session.


PS1, Line 21: amount
number of fsync() operations


PS1, Line 22: plenty
plenty of


PS1, Line 27: rename
renames


PS1, Line 26: to be
            : a
as a


http://gerrit.cloudera.org:8080/#/c/7952/1/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:

Line 1380: // Test available log container selection is FILO.
LIFO here too.


Line 1397:   // Thus, the first available container in the queue can be reused.
"The first available container in the queue will be reused every time."


PS1, Line 1405: finalize
finalizing


http://gerrit.cloudera.org:8080/#/c/7952/1/src/kudu/tserver/tablet_copy_service.cc
File src/kudu/tserver/tablet_copy_service.cc:

PS1, Line 101: FromMilliseconds
Should be FromSeconds.


-- 
To view, visit http://gerrit.cloudera.org:8080/7952
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6eb0bc99422c293417b64e0b6e86c87a3d55e687
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes