You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "song bruce zhang (Code Review)" <ge...@cloudera.org> on 2016/06/02 11:32:51 UTC
[kudu-CR] kudu-1475 set is is initialized to false in log pre-allocated case.
song bruce zhang has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3284
Change subject: kudu-1475 set is is_initialized_ to false in log pre-allocated case.
......................................................................
kudu-1475 set is is_initialized_ to false in log pre-allocated case.
If the log file has been pre-allocated but not initialized
is_initialized_ should be set to false(set as default when initializing
ReadableLogSegment), which is already we documented in annotation.
Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
---
M src/kudu/consensus/log_util.cc
1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/3284/1
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
[kudu-CR] kudu-1475 set is is initialized to false in log pre-allocated case.
Posted by "song bruce zhang (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3284
to look at the new patch set (#2).
Change subject: kudu-1475 set is is_initialized_ to false in log pre-allocated case.
......................................................................
kudu-1475 set is is_initialized_ to false in log pre-allocated case.
If the log file has been pre-allocated but not initialized
is_initialized_ should be set to false(set as default when initializing
ReadableLogSegment), which is already we documented in annotation.
Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
---
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
2 files changed, 7 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/3284/2
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: song bruce zhang <zs...@gmail.com>
[kudu-CR] kudu-1475: set is is initialized to false in log pre-allocated case
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: kudu-1475: set is is_initialized_ to false in log pre-allocated case
......................................................................
Patch Set 3:
(1 comment)
BTW, your tests are failing because this patch is based on one you abandoned that has a typo in column_predicate-test (ASSERT_FLASE() instead of ASSERT_FALSE()).
http://gerrit.cloudera.org:8080/#/c/3284/2//COMMIT_MSG
Commit Message:
Line 7: kudu-1475: set is is_initialized_ to false in log pre-allocated case
> Nit: rewrite as "KUDU-1475: set is_initialized_ to false in log pre-allocat
I think you missed a few of these changes; it should be KUDU-1475 (in capital letters), and "set is is_initialized_" should be "set is_initialized_" (there's an extra "is" in there).
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: song bruce zhang <zs...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] kudu-1475: set is is initialized to false in log pre-allocated case
Posted by "song bruce zhang (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3284
to look at the new patch set (#3).
Change subject: kudu-1475: set is is_initialized_ to false in log pre-allocated case
......................................................................
kudu-1475: set is is_initialized_ to false in log pre-allocated case
If the log file has been pre-allocated but not initialized
is_initialized_ should be set to false(set as default when initializing
ReadableLogSegment), which is already we documented in annotation.
Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
---
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
2 files changed, 7 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/3284/3
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: song bruce zhang <zs...@gmail.com>
[kudu-CR] kudu-1475 set is is initialized to false in log pre-allocated case.
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: kudu-1475 set is is_initialized_ to false in log pre-allocated case.
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/1716/
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] kudu-1475: set is is initialized to false in log pre-allocated case
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has abandoned this change.
Change subject: kudu-1475: set is is_initialized_ to false in log pre-allocated case
......................................................................
Abandoned
The JIRA was closed a long time back, can't repro in recent versions
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: song bruce zhang <zs...@gmail.com>
[kudu-CR] kudu-1475: set is is initialized to false in log pre-allocated case
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: kudu-1475: set is is_initialized_ to false in log pre-allocated case
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3284/3/src/kudu/consensus/log_reader.cc
File src/kudu/consensus/log_reader.cc:
Line 150: VLOG(WARNING) << "Uninitialized segment at: " << segment->path();
VLOG(WARNING) doesn't really exist (surprising it compiles). Should be VLOG(1) probably. Or, if we want it to be a warning that the user sees, I think it should probably be LOG(INFO) and contain an extra sentence about what this actually means (i.e that the server probably crashed in between creating this log and writing the header)
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: song bruce zhang <zs...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] kudu-1475 set is is initialized to false in log pre-allocated case.
Posted by "song bruce zhang (Code Review)" <ge...@cloudera.org>.
song bruce zhang has posted comments on this change.
Change subject: kudu-1475 set is is_initialized_ to false in log pre-allocated case.
......................................................................
Patch Set 1:
@todd, add logic here maybe better , as it is in line with code comments in ReadableLogSegment::ReadHeader . Of course modification as you have described in slack will work too.
diff --git a/src/kudu/consensus/log_reader.cc b/src/kudu/consensus/log_reader.cc
index c7d001d..ab1af5a 100644
--- a/src/kudu/consensus/log_reader.cc
+++ b/src/kudu/consensus/log_reader.cc
@@ -147,6 +147,9 @@ Status LogReader::Init(const string& tablet_wal_path) {
"Unable to open readable log segment");
DCHECK(segment);
CHECK(segment->IsInitialized()) << "Uninitialized segment at: " << segment->path();
+ if (!segment->HasHeader()) {
+ continue;
+ }
if (!segment->HasFooter()) {
LOG(INFO) << "Log segment " << fqp << " was likely left in-progress "
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: song bruce zhang <zs...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] kudu-1475 set is is initialized to false in log pre-allocated case.
Posted by "song bruce zhang (Code Review)" <ge...@cloudera.org>.
song bruce zhang has posted comments on this change.
Change subject: kudu-1475 set is is_initialized_ to false in log pre-allocated case.
......................................................................
Patch Set 1:
> @todd, add logic here maybe better , as it is in line with code
> comments in ReadableLogSegment::ReadHeader .
> Of course modification you have described in slack will work too.
> diff --git a/src/kudu/consensus/log_reader.cc b/src/kudu/consensus/log_reader.cc
> index c7d001d..ab1af5a 100644
> --- a/src/kudu/consensus/log_reader.cc
> +++ b/src/kudu/consensus/log_reader.cc
> @@ -147,6 +147,9 @@ Status LogReader::Init(const string&
> tablet_wal_path) {
> "Unable to open readable log segment");
> DCHECK(segment);
> CHECK(segment->IsInitialized()) << "Uninitialized segment at: " <<
> segment->path();
> + if (!segment->HasHeader()) {
> + continue;
> + }
>
> if (!segment->HasFooter()) {
> LOG(INFO) << "Log segment " << fqp << " was likely left in-progress
> "
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: song bruce zhang <zs...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] kudu-1475 set is is initialized to false in log pre-allocated case.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: kudu-1475 set is is_initialized_ to false in log pre-allocated case.
......................................................................
Patch Set 2:
(1 comment)
Makes sense. What exactly will happen to these preallocated but uninitialized log segments? Do they remain on-disk forever? Or do they get cleaned up eventually?
http://gerrit.cloudera.org:8080/#/c/3284/2//COMMIT_MSG
Commit Message:
Line 7: kudu-1475 set is is_initialized_ to false in log pre-allocated case.
Nit: rewrite as "KUDU-1475: set is_initialized_ to false in log pre-allocated case"
(no terminating period, KUDU in all caps when referencing a JIRA, and extra 'is')
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: song bruce zhang <zs...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] kudu-1475 set is is initialized to false in log pre-allocated case.
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: kudu-1475 set is is_initialized_ to false in log pre-allocated case.
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/1717/
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: song bruce zhang <zs...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] kudu-1475: set is is initialized to false in log pre-allocated case
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: kudu-1475: set is is_initialized_ to false in log pre-allocated case
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/1727/
--
To view, visit http://gerrit.cloudera.org:8080/3284
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45b2beb7c669b1821be8242f45fcea4c6ae269a0
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: song bruce zhang <zs...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: song bruce zhang <zs...@gmail.com>
Gerrit-HasComments: No