[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"

2016-11-25 Thread Maxim Smyatkin (Code Review) 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"

2016-11-25 Thread Maxim Smyatkin (Code Review) 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"

2016-11-18 Thread Maxim Smyatkin (Code Review) 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1634 (part 2). Removed redundant check for tmp log segments.

2016-11-18 Thread Maxim Smyatkin (Code Review) 
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.newsegmentXX, 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin

[kudu-CR] KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments

2016-11-18 Thread Maxim Smyatkin (Code Review) 
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.newsegmentXX files won't pass the previous c
I couldn't find why there would exist any tmp files except the 
.tmp.newsegmentXX 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments

2016-11-17 Thread Maxim Smyatkin (Code Review) 
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.newsegmentXX 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"

2016-11-17 Thread Maxim Smyatkin (Code Review) 
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

[kudu-CR] KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments

2016-11-16 Thread Maxim Smyatkin (Code Review) 
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:
.tmp.newsegmentXX, 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

[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup

2016-11-15 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has posted comments on this change.

Change subject: KUDU-1634. TS and master should delete tmp metadata files on 
startup
..


Patch Set 5:

Having more specific tmp infix seems good.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup

2016-11-15 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has posted comments on this change.

Change subject: KUDU-1634. TS and master should delete tmp metadata files on 
startup
..


Patch Set 5:

I guess it would need Kudu servers to be executed by someone like root and it 
will only remove .tmp files in this case.
But it's your call :) I suppose we can check relative paths, so that symlink 
doesn't go anywhere outside kudu fs root paths, if it's really an issue.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup

2016-11-15 Thread Maxim Smyatkin (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1634. TS and master should delete tmp metadata files on 
startup
..

KUDU-1634. TS and master should delete tmp metadata files on startup

Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
block manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).

Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 154 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/5007/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin

[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup

2016-11-12 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has posted comments on this change.

Change subject: KUDU-1634. TS and master should delete tmp metadata files on 
startup
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5007/3/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:

Line 224: TEST_F(FsManagerTestBase, TestTmpFilesCleanup) {
> Could you add something to the test file tree to exercise the checked_dirs 
Sure, I can. But how will Jenkins behave if it actually does loop forever? Will 
it stop it and report a failure?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup

2016-11-10 Thread Maxim Smyatkin (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1634. TS and master should delete tmp metadata files on 
startup
..

KUDU-1634. TS and master should delete tmp metadata files on startup

Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
block manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).

Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 138 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup

2016-11-10 Thread Maxim Smyatkin (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1634. TS and master should delete tmp metadata files on 
startup
..

KUDU-1634. TS and master should delete tmp metadata files on startup

Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
block manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).

Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 138 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1634. TS and MS should delete tmp metadata files on startup

2016-11-08 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has uploaded a new change for review.

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

Change subject: KUDU-1634. TS and MS should delete tmp metadata files on startup
..

KUDU-1634. TS and MS should delete tmp metadata files on startup

Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
black manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).

Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 95 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status or void

2016-10-25 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has posted comments on this change.

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status or void
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4800/3//COMMIT_MSG
Commit Message:

PS3, Line 7: Status
> Nit: Do you still want to keep the commit header as Status or change it as 
Yes, probably should be updated


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status or void

2016-10-25 Thread Maxim Smyatkin (Code Review) 
Hello Dinesh Bhat, Kudu Jenkins,

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

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

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

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status or void
..

KUDU-78. Fix pb_util functions which return bool to return Status or void

As suggested in ticket, I've refactored four pb_util functions returning bool.
As it turns out, three of them should never fail (except with CHECKS), so I've
changed these to return void and added one missing check. The return type
change was propogated to callers as well: if any of them can't fail anymore
it's also refactored to return void.
The fourth one (ParseFromSequentialFile) can actually fail in two ways:
either with IO error or with Protobuf error (Status::Corruption). To track
which one, I had to add GetStatus() method into SequentialFileFileInputStream.

Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/mt-log-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/delta_compaction-test.cc
M src/kudu/tablet/delta_compaction.cc
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore.cc
M src/kudu/util/pb_util-internal.h
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
18 files changed, 87 insertions(+), 125 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/4800/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4800
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status

2016-10-24 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has posted comments on this change.

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status
..


Patch Set 1:

(3 comments)

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

Line 9: As suggested in ticket, I've refactored four pb_util functions to 
return Status instead of bool.
> Nit: IIRC typically we keep commit message line wrapped to 80 chars or belo
Ok, didn't know it. Will have to change default git editor from nano to 
something showing column positions :)


http://gerrit.cloudera.org:8080/#/c/4800/1/src/kudu/util/pb_util.cc
File src/kudu/util/pb_util.cc:

Line 431:   return Status::OK();
> Few of these routines do not seem to be returning no other value than Statu
Well, it would be easier for me to do so.
But I thought they were designed to return bools at the first place because 
they were expected to fail in future. And for such cases the error handling 
code was already added.

Maybe I've got it wrong and they can be safely changed to void?


Line 443: return Status::Corruption("Error parsing msg", 
InitializationErrorMessage("parse", *msg));
> This is the only case I don't understand; how could msg->ParseFromZeroCopyS
I got the idea that msg may be malformed or something from TODO in pb_util.h:
"TODO: change this to return Status - differentiate IO error from bad PB"

Perhaps, that's wrong. Let me check it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-78. Fix pb util functions which return bool to return Status

2016-10-22 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has uploaded a new change for review.

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

Change subject: KUDU-78. Fix pb_util functions which return bool to return 
Status
..

KUDU-78. Fix pb_util functions which return bool to return Status

As suggested in ticket, I've refactored four pb_util functions to return Status 
instead of bool.
Only one of them can actually fail, for now: ParseFromSequentialFile. It can 
fail in two wais:
either with IO error or with Protobuf error (Status::Corruption). To track 
which one, I had to add
GetStatus() method into SequentialFileFileInputStream.

Also there were few misprints in master/sys_catalog.cc error messages 
("table"/"tablet"),
and a small bug in consensus/log.cc with wrong string::Substitute format ($1 
instead of $0).

Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log_util.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/util/pb_util-internal.h
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
9 files changed, 48 insertions(+), 51 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib760793f6e6da3e357573e525f47b32c79472468
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin

[kudu-CR] KUDU-1696. Daemons should dump their version info to the INFO log at startup

2016-10-20 Thread Maxim Smyatkin (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1696. Daemons should dump their version info to the INFO 
log at startup
..

KUDU-1696. Daemons should dump their version info to the INFO log at startup

In addition to adding full version info on tserver and master startup,
I've added logging of command line arguments as suggested in the ticket.
Using flags before ParseCommandLineFlags() as defaults, I've been able to filter
out from output any flags set explicitly in source code or values set to their 
defaults
in command line.
Here is what it looks like in ts log now, for example:

I1016 23:17:21.764128 10547 tablet_server_main.cc:55] Tablet server executed 
with:
--fs_data_dirs=/tmp/kudu/5VmpWrWo/ts
--fs_wal_dir=/tmp/kudu/5VmpWrWo/ts
--rpc_bind_addresses=127.0.0.1
--webserver_interface=localhost
--webserver_port=8051
--tserver_master_addrs=127.0.0.1
--heap_profile_path=/tmp/kudu-tserver.7060
--unlock_experimental_flags=true
--local_ip_for_outbound_sockets=127.0.0.1
--log_dir=/tmp/kudu/5VmpWrWo
Tablet server version:
kudu 1.1.0-SNAPSHOT
revision de38975ba220d57f241416b61ac1ecb40f2de75a-dirty
build type RELEASE
built by max at 19 Oct 2016 00:56:30 MSK on destr-workspace

Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
---
M src/kudu/master/master_main.cc
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/test_macros.h
7 files changed, 166 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/4733/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1696. Daemons should dump their version info to the INFO log at startup

2016-10-20 Thread Maxim Smyatkin (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1696. Daemons should dump their version info to the INFO 
log at startup
..

KUDU-1696. Daemons should dump their version info to the INFO log at startup

In addition to adding full version info on tserver and master startup,
I've added logging of command line arguments as suggested in the ticket.
Using flags before ParseCommandLineFlags() as defaults, I've been able to filter
out from output any flags set explicitly in source code or values set to their 
defaults
in command line.
Here is what it looks like in ts log now, for example:

I1016 23:17:21.764128 10547 tablet_server_main.cc:55] Tablet server executed 
with:
--fs_data_dirs=/tmp/kudu/5VmpWrWo/ts
--fs_wal_dir=/tmp/kudu/5VmpWrWo/ts
--rpc_bind_addresses=127.0.0.1
--webserver_interface=localhost
--webserver_port=8051
--tserver_master_addrs=127.0.0.1
--heap_profile_path=/tmp/kudu-tserver.7060
--unlock_experimental_flags=true
--local_ip_for_outbound_sockets=127.0.0.1
--log_dir=/tmp/kudu/5VmpWrWo
Tablet server version:
kudu 1.1.0-SNAPSHOT
revision de38975ba220d57f241416b61ac1ecb40f2de75a-dirty
build type RELEASE
built by max at 19 Oct 2016 00:56:30 MSK on destr-workspace

Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
---
M src/kudu/master/master_main.cc
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/test_macros.h
7 files changed, 167 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/4733/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1696. Daemons should dump their version info to the INFO log at startup

2016-10-19 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has posted comments on this change.

Change subject: KUDU-1696. Daemons should dump their version info to the INFO 
log at startup
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4733/3/src/kudu/util/flags-test.cc
File src/kudu/util/flags-test.cc:

PS3, Line 90: ndefault
> Is this a typo or did you intentionally put an invalid flagfile with 'ndefa
typo, thanks


http://gerrit.cloudera.org:8080/#/c/4733/3/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

PS3, Line 366:  // This only means, that the flag have been rewritten. Doesn't 
mean, that
 :   // it is done in command line or even that it's trully 
different from
 :   // default value.
> some grammar/spelling mistakes:
Ok, thanks for rewriting :)


Line 370:   for (const auto& default_flag : default_flags) {
> rather than the O(n^2) loop here, I think it would be better to capture the
I was thinking about this, but it's done only once at startup anyways and if we 
replace it with a map construction, we will just trade this nested loop for:
 1) creating a map (still having the temporary vector)
 2) inserting into map, i.e. allocating space for flags, making copies and 
looking for the right spot with O(N*logN)
 3) looking them up in map, yet another N*logN.
It's probably not worth doing so unless we have several hundreds of flags. 
Still want me to change it?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1696. Daemons should dump their version info to the INFO log at startup

2016-10-18 Thread Maxim Smyatkin (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1696. Daemons should dump their version info to the INFO 
log at startup
..

KUDU-1696. Daemons should dump their version info to the INFO log at startup

In addition to adding full version info on tserver and master startup,
I've added logging of command line arguments as suggested in the ticket.
Using flags before ParseCommandLineFlags() as defaults, I've been able to filter
out from output any flags set explicitly in source code or values set to their 
defaults
in command line.
Here is what it looks like in ts log now, for example:

I1016 23:17:21.764128 10547 tablet_server_main.cc:55] Tablet server executed 
with:
--fs_data_dirs=/tmp/kudu/5VmpWrWo/ts
--fs_wal_dir=/tmp/kudu/5VmpWrWo/ts
--rpc_bind_addresses=127.0.0.1
--webserver_interface=localhost
--webserver_port=8051
--tserver_master_addrs=127.0.0.1
--heap_profile_path=/tmp/kudu-tserver.7060
--unlock_experimental_flags=true
--local_ip_for_outbound_sockets=127.0.0.1
--log_dir=/tmp/kudu/5VmpWrWo
Tablet server version:
kudu 1.1.0-SNAPSHOT
revision de38975ba220d57f241416b61ac1ecb40f2de75a-dirty
build type RELEASE
built by max at 19 Oct 2016 00:56:30 MSK on destr-workspace

Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
---
M src/kudu/master/master_main.cc
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
6 files changed, 168 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1696. Daemons should dump their version info to the INFO log at startup

2016-10-17 Thread Maxim Smyatkin (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1696. Daemons should dump their version info to the INFO 
log at startup
..

KUDU-1696. Daemons should dump their version info to the INFO log at startup

In addition to adding full version info on tserver and master startup,
I've added logging of command line arguments as suggested in the ticket.
Although the initial idea was to log only flags different from their defaults,
I merely implemented logging of all given command line arguments (after they're
successfully parsed by GFlags).
The reasons are: a) some flags differ from their defaults explicitly,
like FLAG_rpc_bind_addresses in master_main.cc, and b) others do it
that way and it seems good enough, right?
Here is what it looks like in ts log now, for example:

I1016 23:17:21.764128 10547 tablet_server_main.cc:55] Tablet server executed 
with:
--unlock_experimental_flags
--log_dir=/tmp/kudu/whzdP6lK
--fs_wal_dir=/tmp/kudu/whzdP6lK/ts
--fs_data_dirs=/tmp/kudu/whzdP6lK/ts
--rpc_bind_addresses=127.0.0.1
--local_ip_for_outbound_sockets=127.0.0.1
--webserver_interface=localhost
--webserver_port=8051
--tserver_master_addrs=127.0.0.1
Tablet server version:
kudu 1.1.0-SNAPSHOT
revision 0c52f161b4504480adcb9e06b7bb0bc5b3e192b0-dirty
build type RELEASE
built by max at 16 Oct 2016 16:24:01 MSK on destr-workspace

Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
---
M src/kudu/master/master_main.cc
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
6 files changed, 95 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1696. Daemons should dump their version info to the INFO log at startup

2016-10-16 Thread Maxim Smyatkin (Code Review) 
Maxim Smyatkin has uploaded a new change for review.

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

Change subject: KUDU-1696. Daemons should dump their version info to the INFO 
log at startup
..

KUDU-1696. Daemons should dump their version info to the INFO log at startup

In addition to adding full version info on tserver and master startup,
I've added logging of command line arguments as suggested in the ticket.
Although the initial idea was to log only flags different from their defaults,
I merely implemented logging of all given command line arguments (after they're
successfully parsed by GFlags).
The reasons are: a) some flags differ from their defaults explicitly,
like FLAG_rpc_bind_addresses in master_main.cc, and b) others do it
that way and it seems good enough, right?
Here is what it looks like in ts log now, for example:

I1016 23:17:21.764128 10547 tablet_server_main.cc:55] Tablet server executed 
with:
--unlock_experimental_flags
--log_dir=/tmp/kudu/whzdP6lK
--fs_wal_dir=/tmp/kudu/whzdP6lK/ts
--fs_data_dirs=/tmp/kudu/whzdP6lK/ts
--rpc_bind_addresses=127.0.0.1
--local_ip_for_outbound_sockets=127.0.0.1
--webserver_interface=localhost
--webserver_port=8051
--tserver_master_addrs=127.0.0.1
Tablet server version:
kudu 1.1.0-SNAPSHOT
revision 0c52f161b4504480adcb9e06b7bb0bc5b3e192b0-dirty
build type RELEASE
built by max at 16 Oct 2016 16:24:01 MSK on destr-workspace

Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
---
M src/kudu/master/master_main.cc
M src/kudu/tserver/tablet_server_main.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
6 files changed, 95 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I846827c53e74ca364af61c690bf85af8a8f28601
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin