You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org> on 2016/11/17 08:52:07 UTC
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Maxim Smyatkin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5123
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
As these ".tmp" strings were scattered over the code, I've tried to remove
this duplication as well by defining the single kTmpInfix in pb_util.h.
In a couple of places it led to string concatenations, but I suppose it's
not that bad for overal performance?
The necessity of moving from "tmp" to "kudutmp" is backed by the idea that
we don't want to remove any user's data accidentally
(see https://gerrit.cloudera.org/#/c/5007/).
Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_session-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
11 files changed, 31 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/5123/1
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Maxim Smyatkin has posted comments on this change.
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
Patch Set 2:
> I'm not really sold on this being in pb_util, since it's used for
> non-protobuf files too. Maybe path_util is a better spot?
Yes, you're right. Thanks for the hint, I didn't even know about it's existence :)
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
Patch Set 2:
I'm not really sold on this being in pb_util, since it's used for non-protobuf files too. Maybe path_util is a better spot?
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5123
to look at the new patch set (#4).
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
As these ".tmp" strings were scattered over the code, I've tried to remove
this duplication as well by defining the single kTmpInfix in path_util.h.
In a couple of places it led to string concatenations, but I suppose it's
not that bad for overall performance?
The necessity of moving from "tmp" to "kudutmp" is backed by the idea that
we don't want to remove any user's data accidentally. For example, we may
have a symlink to some external directory, which doesn't belong only to
Kudu. Hence, we don't want to delete any ordinary tmp files from it.
(see https://gerrit.cloudera.org/#/c/5007/).
Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
---
M src/kudu/consensus/log.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
M src/kudu/util/pb_util.cc
10 files changed, 25 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/5123/4
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
Patch Set 3:
Hey Maxim, looks like this patch has some conflicts and needs to be rebased.
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5123
to look at the new patch set (#2).
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
As these ".tmp" strings were scattered over the code, I've tried to remove
this duplication as well by defining the single kTmpInfix in pb_util.h.
In a couple of places it led to string concatenations, but I suppose it's
not that bad for overall performance?
The necessity of moving from "tmp" to "kudutmp" is backed by the idea that
we don't want to remove any user's data accidentally. For example, we may
have a symlink to some external directory, which doesn't belong only to
Kudu. Hence, we don't want to delete any ordinary tmp files from it.
(see https://gerrit.cloudera.org/#/c/5007/).
Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
---
M src/kudu/consensus/log.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_session-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
9 files changed, 30 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/5123/2
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
As these ".tmp" strings were scattered over the code, I've tried to remove
this duplication as well by defining the single kTmpInfix in path_util.h.
In a couple of places it led to string concatenations, but I suppose it's
not that bad for overall performance?
The necessity of moving from "tmp" to "kudutmp" is backed by the idea that
we don't want to remove any user's data accidentally. For example, we may
have a symlink to some external directory, which doesn't belong only to
Kudu. Hence, we don't want to delete any ordinary tmp files from it.
(see https://gerrit.cloudera.org/#/c/5007/).
Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Reviewed-on: http://gerrit.cloudera.org:8080/5123
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/consensus/log.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
M src/kudu/util/pb_util.cc
10 files changed, 25 insertions(+), 22 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
Patch Set 4:
> Hey Maxim, looks like this patch has some conflicts and needs to be
> rebased.
Don't worry about it; I rebased the patch.
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5123/1//COMMIT_MSG
Commit Message:
PS1, Line 12: overal
Nit: overall
Line 14: we don't want to remove any user's data accidentally
Nit: to expand on this, note that while Kudu "owns" its data directories, a symlink pointing to the outside world will be followed, and that's when we don't want to rampantly delete any "tmp" file.
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5123
to look at the new patch set (#3).
Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
......................................................................
KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"
As these ".tmp" strings were scattered over the code, I've tried to remove
this duplication as well by defining the single kTmpInfix in path_util.h.
In a couple of places it led to string concatenations, but I suppose it's
not that bad for overall performance?
The necessity of moving from "tmp" to "kudutmp" is backed by the idea that
we don't want to remove any user's data accidentally. For example, we may
have a symlink to some external directory, which doesn't belong only to
Kudu. Hence, we don't want to delete any ordinary tmp files from it.
(see https://gerrit.cloudera.org/#/c/5007/).
Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
---
M src/kudu/consensus/log.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_session-test.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
M src/kudu/util/pb_util.cc
10 files changed, 25 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/5123/3
--
To view, visit http://gerrit.cloudera.org:8080/5123
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>