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 07:50:12 UTC
[kudu-CR] KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments
Maxim Smyatkin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5122
Change subject: KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments
......................................................................
KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments
Temporary consensus log semgents are being named this way:
<name>.tmp.newsegmentXXXXXX, that is .tmp here is infix while IsLogFileName()
looks for a suffix and fails to find it. As a result, temporary segment will
be considered as finished.
Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
---
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
2 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/5122/1
--
To view, visit http://gerrit.cloudera.org:8080/5122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
[kudu-CR] KUDU-1634 (part 2). Removed redundant check for tmp log segments.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: KUDU-1634 (part 2). Removed redundant check for tmp log segments.
......................................................................
KUDU-1634 (part 2). Removed redundant check for tmp log segments.
Temporary consensus log semgents are being named this way:
.tmp.newsegmentXXXXXX, meaning they are already being ignorred by
HasPrefixString(fname, ".") check for hidden files and "." or "..".
So, there is actually no reason to look for tmp files specifically.
Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
Reviewed-on: http://gerrit.cloudera.org:8080/5122
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
2 files changed, 0 insertions(+), 10 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved; Verified
Todd Lipcon: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/5122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
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: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1634 (part 2). Removed redundant check for tmp log segments.
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/5122
to look at the new patch set (#2).
Change subject: KUDU-1634 (part 2). Removed redundant check for tmp log segments.
......................................................................
KUDU-1634 (part 2). Removed redundant check for tmp log segments.
Temporary consensus log semgents are being named this way:
.tmp.newsegmentXXXXXX, meaning they are already being ignorred by
HasPrefixString(fname, ".") check for hidden files and "." or "..".
So, there is actually no reason to look for tmp files specifically.
Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
---
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
2 files changed, 0 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/5122/2
--
To view, visit http://gerrit.cloudera.org:8080/5122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
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>
[kudu-CR] KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Maxim Smyatkin has posted comments on this change.
Change subject: KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5122/1/src/kudu/consensus/log_util.cc
File src/kudu/consensus/log_util.cc:
Line 810: if (fname.find(kTmpInfix) != fname.npos) {
> If I'm understanding the code in Log::CreatePlaceholderSegment() correctly,
Hm, you're right, the .tmp.newsegmentXXXXXX files won't pass the previous check.
But whether there may be any different tmp files or not, I'm not sure. Maybe the whole check is really unnecessary. Will check it later.
--
To view, visit http://gerrit.cloudera.org:8080/5122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
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-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1634 (part 2). Removed redundant check for tmp log segments.
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1634 (part 2). Removed redundant check for tmp log segments.
......................................................................
Patch Set 2: Code-Review+1
Seems reasonable to me. I wonder if we should be removing these tmps, though. Does that come in the next patch?
--
To view, visit http://gerrit.cloudera.org:8080/5122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
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 2). Fixed bug which fails to ignore tmp consensus log segments
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Maxim Smyatkin has posted comments on this change.
Change subject: KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5122/1/src/kudu/consensus/log_util.cc
File src/kudu/consensus/log_util.cc:
Line 810: if (fname.find(kTmpInfix) != fname.npos) {
> Hm, you're right, the .tmp.newsegmentXXXXXX files won't pass the previous c
I couldn't find why there would exist any tmp files except the .tmp.newsegmentXXXXXX ones.
--
To view, visit http://gerrit.cloudera.org:8080/5122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
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-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1634 (part 2). Removed redundant check for tmp log segments.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634 (part 2). Removed redundant check for tmp log segments.
......................................................................
Patch Set 2: Verified+1
Overriding Jenkins, failure was KUDU-1624.
--
To view, visit http://gerrit.cloudera.org:8080/5122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
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 2). Fixed bug which fails to ignore tmp consensus log segments
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments
......................................................................
Patch Set 1:
(1 comment)
Agreed that the TSAN failure is not due to your change. You can ignore it.
http://gerrit.cloudera.org:8080/#/c/5122/1/src/kudu/consensus/log_util.cc
File src/kudu/consensus/log_util.cc:
Line 810: if (fname.find(kTmpInfix) != fname.npos) {
If I'm understanding the code in Log::CreatePlaceholderSegment() correctly, the temp files will be created with the name ".tmp.newsegmentXXXXXX" where XXXXXX has been replaced by mkstemp() in some way. So maybe we don't need this check at all, since the HasPrefixString() on L804 will find these "hidden" files?
Or is this check intended to capture other temp files rather than just temporary log segments?
--
To view, visit http://gerrit.cloudera.org:8080/5122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
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 2). Removed redundant check for tmp log segments.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634 (part 2). Removed redundant check for tmp log segments.
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5122
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
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-HasComments: No