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/08/24 00:48:23 UTC

[kudu-CR] log block manager: use unsigned int for next block id

Hao Hao has uploaded a new change for review.

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

Change subject: log block manager: use unsigned int for next_block_id_
......................................................................

log block manager: use unsigned int for next_block_id_

next_block_id_ is used to keep track of which unique block id
should be used for block creation. Currently, it is defined as int64_t.
However, it could be updated based on the value of 'max_block_id'
which is uint64_t.

This patch changes the type of next_block_id_ to uint64_t to
avoid overflow due to conversation of unsinged int to int, which
can result in block id reused.

Change-Id: Ib315b20719ef529331304df5c56c4242902524d4
---
M src/kudu/fs/log_block_manager.h
1 file changed, 1 insertion(+), 1 deletion(-)


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

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

[kudu-CR] log block manager: use unsigned int for next block id

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change.

Change subject: log block manager: use unsigned int for next_block_id_
......................................................................

Patch Set 1:

(3 comments)

I've been wondering whether this deserves a standalone test. I'm inclined to say 'no' since I think the pain is only felt in specific test-only scenarios. Namely:
1) You have to be running in a gtest, so either you're using an InternalMiniCluster or a lower level unit test.
2) You have to be using the block cache singleton.
3) You have to flush some data.
4) You have to restart the block manager some number of times.

BTW, KUDU-1538 is related and perhaps should be referenced in the commit description.

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

Line 14: This patch changes the type of next_block_id_ to uint64_t to
Should probably explain why we're switching it to uint64_t instead of switching every other site to int64_t (since the convention in Kudu is to use int64_t for large integer values).

The reason is that block IDs are defined as uint64_t both on disk (fs.proto) and in memory (block_id.h), so it makes more sense to treat them as such in the LBM than to convert them properly.

PS1, Line 15: conversation
conversion

PS1, Line 16: in block id reused.
in the reuse of an existing block ID.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib315b20719ef529331304df5c56c4242902524d4
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-HasComments: Yes