You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Jeffrey F. Lukman (Code Review)" <ge...@cloudera.org> on 2018/01/18 23:42:29 UTC

[kudu-CR] Kudu-2126: Add conditional check to prevent unnecessary fsyncs 

Jeffrey F. Lukman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9069


Change subject: Kudu-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................

Kudu-2126: Add conditional check to prevent unnecessary fsyncs

This patch includes unit test to detect fsyncs that happened
when a DeleteTablet RPC tries to tombstone an already tombstoned tablet.
In order to fix the issue, this patch add additional checking
before executing another DeleteTabletData that leads to another fsyncs.

Furthermore, this patch also refine the TABLET_DATA_DELETED comment.

Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
---
M src/kudu/integration-tests/delete_tablet-itest.cc
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/ts_tablet_manager.cc
5 files changed, 75 insertions(+), 3 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 1
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>

[kudu-CR] KUDU-2126: Add conditional check to prevent unnecessary fsyncs

Posted by "Jeffrey F. Lukman (Code Review)" <ge...@cloudera.org>.
Jeffrey F. Lukman has posted comments on this change. ( http://gerrit.cloudera.org:8080/9069 )

Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................


Patch Set 3:

> Quick question: did you try running this test without the fix? Does
 > the test fail?

Yes, I did. Without the fix, the test failed.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 3
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Jan 2018 03:50:38 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2126: Add conditional check to prevent unnecessary fsyncs

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

Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................


Patch Set 3:

> Yes, I did. Without the fix, the test failed.

Perfect.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 3
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Jan 2018 03:54:50 +0000
Gerrit-HasComments: No

[kudu-CR] Kudu-2126: Add conditional check to prevent unnecessary fsyncs

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

Change subject: Kudu-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................


Patch Set 1:

(6 comments)

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

http://gerrit.cloudera.org:8080/#/c/9069/1//COMMIT_MSG@7
PS1, Line 7: Kudu-2126: Add conditional check to prevent unnecessary fsyncs
nit: we always capitalize jira names like KUDU-2126


http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/integration-tests/delete_tablet-itest.cc
File src/kudu/integration-tests/delete_tablet-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/integration-tests/delete_tablet-itest.cc@168
PS1, Line 168: // Kudu-2126 : Ensure that DeleteTablet RPCs does not do double fsyncs.
nit: same thing about capitalization


http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/integration-tests/delete_tablet-itest.cc@183
PS1, Line 183: table
tablets


http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/tablet/metadata.proto
File src/kudu/tablet/metadata.proto:

http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/tablet/metadata.proto@66
PS1, Line 66:   // A tablet state is set to TABLET_DATA_DELETED when a tablet is permanently deleted
            :   // from all tablet servers and this state is a terminal state.
            :   // Therefore, the tablet should not be allowed to transition back to any other state.
            :  
I think the bit about it being a "roll-forward" state is useful. Particularly, I think we do flush it to disk as 'deleted' while we are doing the deletion of blocks, right?


http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/tablet/tablet_metadata.cc@528
PS1, Line 528:   flush_count_for_tests_++;
seems we should only increment this down below where we actually do the flush. That way it's protected by the lock l_flush and also it ensures that we don't overcount the case when the metadata has been pinned


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

http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/tserver/ts_tablet_manager.cc@797
PS1, Line 797:   // Kudu-2126 : if tablet is already tombstoned, then a request to tombstone
nit: same formatting



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 1
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 01:03:01 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2126: Add conditional check to prevent unnecessary fsyncs

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

Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................

KUDU-2126: Add conditional check to prevent unnecessary fsyncs

This patch includes a unit test to detect fsyncs that happened
when a DeleteTablet RPC tries to tombstone an already tombstoned tablet.
In order to fix the issue, this patch adds additional checking
before executing another DeleteTabletData that leads to additional fsyncs.

Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Reviewed-on: http://gerrit.cloudera.org:8080/9069
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/integration-tests/delete_tablet-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/ts_tablet_manager.cc
4 files changed, 69 insertions(+), 0 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified
  Todd Lipcon: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 4
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2126: Add conditional check to prevent unnecessary fsyncs

Posted by "Jeffrey F. Lukman (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Todd Lipcon, 

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

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

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

Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................

KUDU-2126: Add conditional check to prevent unnecessary fsyncs

This patch includes a unit test to detect fsyncs that happened
when a DeleteTablet RPC tries to tombstone an already tombstoned tablet.
In order to fix the issue, this patch adds additional checking
before executing another DeleteTabletData that leads to additional fsyncs.

Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
---
M src/kudu/integration-tests/delete_tablet-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/ts_tablet_manager.cc
4 files changed, 69 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/9069/3
-- 
To view, visit http://gerrit.cloudera.org:8080/9069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 3
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2126: Add conditional check to prevent unnecessary fsyncs

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

Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG@9
PS2, Line 9:  
nit: a unit test


http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG@11
PS2, Line 11: add
nit: adds


http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG@12
PS2, Line 12: another
nit: additional


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/integration-tests/delete_tablet-itest.cc
File src/kudu/integration-tests/delete_tablet-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/integration-tests/delete_tablet-itest.cc@169
PS2, Line 169: KUDU-2126 : Ensure that DeleteTablet RPCs does not do double fsyncs.
how about:

  // Regression test for KUDU-2126: Ensure that a DeleteTablet() RPC on a tombstoned tablet does not cause any fsyncs.


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/integration-tests/delete_tablet-itest.cc@191
PS2, Line 191:   ASSERT_EVENTUALLY([&] {
Is this test flaky if you remove this ASSERT_EVENTUALLY? I don't think this is required because workload.Setup() should not return until the tablet has been created.


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:

http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.h@379
PS2, Line 379:   int flush_count_for_tests_;
This seems fine, but I wonder if you considered creating a metric for # of fsyncs.


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.cc@554
PS2, Line 554:   flush_count_for_tests_++;
I think this line should go inside TabletMetadata::ReplaceSuperBlockUnlocked() since that is where the actual flush is taking place. The flush occurs during the call to pb_util::WritePBContainerToPath().


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tserver/ts_tablet_manager.cc@797
PS2, Line 797: KUDU-2126 : i
No need to mention KUDU-2126 here. It should be mentioned in the regression test, instead.


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tserver/ts_tablet_manager.cc@798
PS2, Line 798: p
style nit: end comments with punctuation (i.e. a period). We follow the Google C++ style guide and this guideline can be found at https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 2
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Jan 2018 01:57:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] Kudu-2126: Add conditional check to prevent unnecessary fsyncs

Posted by "Jeffrey F. Lukman (Code Review)" <ge...@cloudera.org>.
Jeffrey F. Lukman has posted comments on this change. ( http://gerrit.cloudera.org:8080/9069 )

Change subject: Kudu-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/tablet/metadata.proto
File src/kudu/tablet/metadata.proto:

http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/tablet/metadata.proto@66
PS1, Line 66:   // A tablet state is set to TABLET_DATA_DELETED when a tablet is permanently deleted
            :   // from all tablet servers and this state is a terminal state.
            :   // Therefore, the tablet should not be allowed to transition back to any other state.
            :  
> I think the bit about it being a "roll-forward" state is useful. Particular
I will remove this change for now.
If this change is still needed, Mike will submit a separate change later.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 1
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 01:30:21 +0000
Gerrit-HasComments: Yes

[kudu-CR] Kudu-2126: Add conditional check to prevent unnecessary fsyncs

Posted by "Jeffrey F. Lukman (Code Review)" <ge...@cloudera.org>.
Jeffrey F. Lukman has posted comments on this change. ( http://gerrit.cloudera.org:8080/9069 )

Change subject: Kudu-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................


Patch Set 1:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/9069/1//COMMIT_MSG@7
PS1, Line 7: Kudu-2126: Add conditional check to prevent unnecessary fsyncs
> nit: we always capitalize jira names like KUDU-2126
Done


http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/integration-tests/delete_tablet-itest.cc
File src/kudu/integration-tests/delete_tablet-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/integration-tests/delete_tablet-itest.cc@168
PS1, Line 168: // Kudu-2126 : Ensure that DeleteTablet RPCs does not do double fsyncs.
> nit: same thing about capitalization
Done


http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/integration-tests/delete_tablet-itest.cc@183
PS1, Line 183: table
> tablets
Done


http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/tablet/tablet_metadata.cc@528
PS1, Line 528:   flush_count_for_tests_++;
> seems we should only increment this down below where we actually do the flu
Done


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

http://gerrit.cloudera.org:8080/#/c/9069/1/src/kudu/tserver/ts_tablet_manager.cc@797
PS1, Line 797:   // Kudu-2126 : if tablet is already tombstoned, then a request to tombstone
> nit: same formatting
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 1
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Jan 2018 01:18:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2126: Add conditional check to prevent unnecessary fsyncs

Posted by "Jeffrey F. Lukman (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Todd Lipcon, 

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

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

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

Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................

KUDU-2126: Add conditional check to prevent unnecessary fsyncs

This patch includes unit test to detect fsyncs that happened
when a DeleteTablet RPC tries to tombstone an already tombstoned tablet.
In order to fix the issue, this patch add additional checking
before executing another DeleteTabletData that leads to another fsyncs.

Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
---
M src/kudu/integration-tests/delete_tablet-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tserver/ts_tablet_manager.cc
4 files changed, 70 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 2
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2126: Add conditional check to prevent unnecessary fsyncs

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

Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................


Patch Set 3:

Quick question: did you try running this test without the fix? Does the test fail?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 3
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Jan 2018 03:05:01 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2126: Add conditional check to prevent unnecessary fsyncs

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

Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 3
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 22 Jan 2018 23:24:45 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2126: Add conditional check to prevent unnecessary fsyncs

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

Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

Looks good, thanks for the quick turnaround!

http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:

http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.h@379
PS2, Line 379:   int flush_count_for_tests_;
> I did, but this method is just easier to implement
That's fair. SGTM.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 3
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Jan 2018 03:03:59 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2126: Add conditional check to prevent unnecessary fsyncs

Posted by "Jeffrey F. Lukman (Code Review)" <ge...@cloudera.org>.
Jeffrey F. Lukman has posted comments on this change. ( http://gerrit.cloudera.org:8080/9069 )

Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs
......................................................................


Patch Set 3:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG@9
PS2, Line 9:  
> nit: a unit test
Done


http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG@11
PS2, Line 11: add
> nit: adds
Done


http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG@12
PS2, Line 12: additio
> nit: additional
Done


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/integration-tests/delete_tablet-itest.cc
File src/kudu/integration-tests/delete_tablet-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/integration-tests/delete_tablet-itest.cc@169
PS2, Line 169: Regression test for KUDU-2126 : Ensure that a DeleteTablet() RPC
> how about:
Done


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/integration-tests/delete_tablet-itest.cc@191
PS2, Line 191:   scoped_refptr<TabletReplica> tablet_replica;
> Is this test flaky if you remove this ASSERT_EVENTUALLY? I don't think this
Done


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:

http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.h@379
PS2, Line 379:   int flush_count_for_tests_;
> This seems fine, but I wonder if you considered creating a metric for # of 
I did, but this method is just easier to implement
and yet it achieves the goal that the unit test wants to do.


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.cc@554
PS2, Line 554:   l_flush.Unlock();
> I think this line should go inside TabletMetadata::ReplaceSuperBlockUnlocke
Done


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tserver/ts_tablet_manager.cc@797
PS2, Line 797: If a tablet i
> No need to mention KUDU-2126 here. It should be mentioned in the regression
Done


http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tserver/ts_tablet_manager.cc@798
PS2, Line 798: 
> style nit: end comments with punctuation (i.e. a period). We follow the Goo
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e
Gerrit-Change-Number: 9069
Gerrit-PatchSet: 3
Gerrit-Owner: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Jeffrey F. Lukman <je...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Jan 2018 02:18:07 +0000
Gerrit-HasComments: Yes