[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8325 )

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..

Integrate BlockCreationTransaction into MajorDeltaCompaction

Previous commit 59ca360bf integrated BlockCreationTransaction with
RowSet compaction, so that only a single transaction is used to finalize
underlying blocks. However in major delta compaction, MultiColumnWriter
and DeltaFileWriter use separate creation transactions, which does not
take full advantage of BlockCreationTransaction.

This patch includes a minor improvement to incorporate
BlockCreationTransaction with MajorDeltaCompaction to use a single
transaction for a RowSet's major delta compaction.

Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Reviewed-on: http://gerrit.cloudera.org:8080/8325
Reviewed-by: Adar Dembo 
Tested-by: Adar Dembo 
---
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
3 files changed, 8 insertions(+), 16 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved; Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Adar Dembo (Code Review)]

Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/8325 )

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8325
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8325 )

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..


Patch Set 3: Verified+1

Agreed.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 20 Oct 2017 00:40:21 +
Gerrit-HasComments: No

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8325 )

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..


Patch Set 3:

> Build Failed
 >
 > http://jenkins.kudu.apache.org/job/kudu-gerrit/10441/ : FAILURE
I don't think the change would cause these tests to fail.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 20 Oct 2017 00:35:04 +
Gerrit-HasComments: No

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8325 )

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..


Patch Set 3: Code-Review+2

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8325/1/src/kudu/tablet/delta_compaction.cc@235
PS1, Line 235:   
RETURN_NOT_OK(base_data_writer_->FinishAndReleaseBlocks(transaction.get()));
> There is no any cases yet but would it be useful in future to use the same
Oh, I see. I suppose it might be useful.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 19 Oct 2017 22:22:57 +
Gerrit-HasComments: Yes

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8325 )

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..


Patch Set 3:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8325/1/src/kudu/tablet/delta_compaction.cc@235
PS1, Line 235:   
RETURN_NOT_OK(base_data_writer_->FinishAndReleaseBlocks(transaction.get()));
> Would be nice to update the transaction classes to prevent errors like this
There is no any cases yet but would it be useful in future to use the same 
BlockCreationTransaction multiple times in unit test like we do for 
BlockDeletionTransaction?


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

http://gerrit.cloudera.org:8080/#/c/8325/2/src/kudu/tablet/multi_column_writer.cc@41
PS2, Line 41: using fs::CreateBlockOptions;
> Could probably drop this now.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 19 Oct 2017 22:17:18 +
Gerrit-HasComments: Yes

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8325 )

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8325/1/src/kudu/tablet/delta_compaction.cc@235
PS1, Line 235:   
RETURN_NOT_OK(base_data_writer_->FinishAndReleaseBlocks(transaction.get()));
> Right... that defeats the purpose of the change. Thanks for catching it! Up
Would be nice to update the transaction classes to prevent errors like this 
(i.e. DCHECK that we aren't committing a transaction multiple times).



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 19 Oct 2017 21:28:46 +
Gerrit-HasComments: Yes

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8325 )

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..


Patch Set 2: Code-Review+2

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8325/2/src/kudu/tablet/multi_column_writer.cc@41
PS2, Line 41: using fs::BlockManager;
Could probably drop this now.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 19 Oct 2017 21:28:11 +
Gerrit-HasComments: Yes

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8325 )

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8325/1//COMMIT_MSG@15
PS1, Line 15: improvemen
> improvement
Done


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

http://gerrit.cloudera.org:8080/#/c/8325/1/src/kudu/tablet/delta_compaction.cc@235
PS1, Line 235:   
RETURN_NOT_OK(base_data_writer_->FinishAndReleaseBlocks(transaction.get()));
> Doesn't each of these FinishWithTransaction() calls commit the transaction
Right... that defeats the purpose of the change. Thanks for catching it! 
Updated.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 19 Oct 2017 21:22:32 +
Gerrit-HasComments: Yes

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Hao Hao (Code Review)]

Hello Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..

Integrate BlockCreationTransaction into MajorDeltaCompaction

Previous commit 59ca360bf integrated BlockCreationTransaction with
RowSet compaction, so that only a single transaction is used to finalize
underlying blocks. However in major delta compaction, MultiColumnWriter
and DeltaFileWriter use separate creation transactions, which does not
take full advantage of BlockCreationTransaction.

This patch includes a minor improvement to incorporate
BlockCreationTransaction with MajorDeltaCompaction to use a single
transaction for a RowSet's major delta compaction.

Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
---
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
3 files changed, 8 insertions(+), 15 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8325 )

Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8325/1//COMMIT_MSG@15
PS1, Line 15: improvment
improvement


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

http://gerrit.cloudera.org:8080/#/c/8325/1/src/kudu/tablet/delta_compaction.cc@235
PS1, Line 235:   
RETURN_NOT_OK(base_data_writer_->FinishWithTransaction(transaction.get()));
Doesn't each of these FinishWithTransaction() calls commit the transaction 
though? Meaning we end up calling commit multiple times? That should be an 
error I would think.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 19 Oct 2017 00:24:35 +
Gerrit-HasComments: Yes

[kudu-CR] Integrate BlockCreationTransaction into MajorDeltaCompaction

[Hao Hao (Code Review)]

Hao Hao has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8325


Change subject: Integrate BlockCreationTransaction into MajorDeltaCompaction
..

Integrate BlockCreationTransaction into MajorDeltaCompaction

Previous commit 59ca360bf integrated BlockCreationTransaction with
RowSet compaction, so that only a single transaction is used to finalize
underlying blocks. However in major delta compaction, MultiColumnWriter
and DeltaFileWriter use separate creation transactions, which does not
take full advantage of BlockCreationTransaction.

This patch includes a minor improvment to incorporate
BlockCreationTransaction with MajorDeltaCompaction to use a single
transaction for a RowSet's major delta compaction.

Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
---
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
5 files changed, 21 insertions(+), 9 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I887e20643ac8e4e728f8f91450d2d5059cb33d20
Gerrit-Change-Number: 8325
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao