You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2017/08/25 03:08:08 UTC
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Hello Dan Burkert, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7826
to review the following change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
catalog_manager: don't log deleted tables/tablets at startup
On long-lived clusters this list can get quite long.
While I was in the area, I made some other cosmetic improvements:
- Applying strings::Substitute() to perforated LOG statements.
- Replacing 'OVERRIDE' with 'override'.
- Removing 'virtual' from overriden methods.
- Removing unnecessary std:: prefixes.
- A few other miscellaneous tweaks.
Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/sys_catalog.h
2 files changed, 191 insertions(+), 205 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/7826/1
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7826/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS2, Line 359: VLOG(2)
> That's the way the old code was; can't say for sure since I didn't write it
It's still 2 but I think it's OK: probably, the idea was to get tablet-related information only at higher debug levels.
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 1:
(3 comments)
How confident are we that the Substitutes won't be more eagerly evaluated than the << operations?
http://gerrit.cloudera.org:8080/#/c/7826/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 1308: LOG(INFO) << Substitute("Servicing CreateTable request from $0: $1",
The previous version has a newline before the table descriptor. I've always found these messages obnoxiously verbose, but I think as long as we're using SecureDebugString and not the Short variant, it's best to keep the leading newline.
Line 2357: VLOG(2) << Substitute("Received tablet report from $0: $1",
Might be good to put a leading newline on the tablet report here for the same reason.
PS1, Line 3133: deleted
I think it was correct before
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 2:
Interesting about the naming conflict. I'm surprised that ASAN's ODR violation checker didn't spot this.
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 2:
(8 comments)
> How confident are we that the Substitutes won't be more eagerly
> evaluated than the << operations?
We discussed this offline. I wrote a test that evaluated this code:
VLOG(1) << Substitute("abcd $0", [&]() {
CHECK(false);
return string("12345");
}());
When run normally, it passed. When run with -v=2, it aborted.
So it appears that glog is lazily evaluating the entirety of the expression after a VLOG.
http://gerrit.cloudera.org:8080/#/c/7826/2//COMMIT_MSG
Commit Message:
PS2, Line 14: overriden
> nit: overridden
Done
http://gerrit.cloudera.org:8080/#/c/7826/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 1308: LOG(INFO) << Substitute("Servicing CreateTable request from $0: $1",
> The previous version has a newline before the table descriptor. I've alway
I did this to be consistent with AlterTable/DeleteTable, but I don't mind a newline for all three.
Line 2357: VLOG(2) << Substitute("Received tablet report from $0: $1",
> Might be good to put a leading newline on the tablet report here for the sa
Done
PS1, Line 3133: deleted
> I think it was correct before
Whoops.
http://gerrit.cloudera.org:8080/#/c/7826/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS2, Line 359: VLOG(2)
> interesting: for tables the debug log level is 1. Is this intentional to h
That's the way the old code was; can't say for sure since I didn't write it (I don't think).
But I'll put them at the same log level.
PS2, Line 2616: WARNING
> Maybe, it's worth set to up ERROR since this is an error condition and the
Done
PS2, Line 2951: ...
> nit: drop the ellipsis?
Done
PS2, Line 2947: if (delay_millis <= 0) {
: LOG(WARNING) << "Request timed out: " << description();
: MarkFailed();
: } else {
: LOG(INFO) << Substitute("Scheduling retry of $0 with a delay of $1 ms (attempt = $2)...",
: description(), delay_millis, attempt_);
: master_->messenger()->ScheduleOnReactor(
: boost::bind(&RetryingTSRpcTask::RunDelayedTask, this, _1),
: MonoDelta::FromMilliseconds(delay_millis));
: return true;
: }
: return false;
> nit: while you are at it, consider removing the extra-scope for better read
Done
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 4: Verified+1
Overriding Jenkins, another Gradle daemon failure.
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
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/7826
to look at the new patch set (#2).
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
catalog_manager: don't log deleted tables/tablets at startup
On long-lived clusters this list can get quite long.
While I was in the area, I made some other cosmetic improvements:
- Applying strings::Substitute() to perforated LOG statements.
- Replacing 'OVERRIDE' with 'override'.
- Removing 'virtual' from overriden methods.
- Removing unnecessary std:: prefixes.
- Adding virtual destructors to {Table,Tablet}Visitor.
- A few other miscellaneous tweaks.
The new virtual destructors caused sys_catalog-test to fail in a bizarre
way: the catalog manager ran ~TabletLoader defined in sys_catalog-test.cc
rather than the implicit one from catalog_manager.cc. I fixed this by
renaming the sys_catalog-test {Table,Tablet}Loader classes; not quite sure
why there was no collision at link-time in the first place.
Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.h
3 files changed, 199 insertions(+), 213 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/7826/2
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 3: Verified+1
Overriding Jenkins, another Gradle daemon failure.
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
catalog_manager: don't log deleted tables/tablets at startup
On long-lived clusters this list can get quite long.
While I was in the area, I made some other cosmetic improvements:
- Applying strings::Substitute() to perforated LOG statements.
- Replacing 'OVERRIDE' with 'override'.
- Removing 'virtual' from overridden methods.
- Removing unnecessary std:: prefixes.
- Adding virtual destructors to {Table,Tablet}Visitor.
- A few other miscellaneous tweaks.
The new virtual destructors caused sys_catalog-test to fail in a bizarre
way: the catalog manager ran ~TabletLoader defined in sys_catalog-test.cc
rather than the implicit one from catalog_manager.cc. I fixed this by
renaming the sys_catalog-test {Table,Tablet}Loader classes; not quite sure
why there was no collision at link-time in the first place.
Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Reviewed-on: http://gerrit.cloudera.org:8080/7826
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.h
3 files changed, 204 insertions(+), 219 deletions(-)
Approvals:
Adar Dembo: Verified
Todd Lipcon: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7826/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS2, Line 359: VLOG(2)
> It's still 2 but I think it's OK: probably, the idea was to get tablet-rela
Right, rather than drop VisitTablet to 1 I raised VisitTable() to 2. I think 1 is too verbose. for this information.
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Alexey Serbin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7826
to look at the new patch set (#4).
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
catalog_manager: don't log deleted tables/tablets at startup
On long-lived clusters this list can get quite long.
While I was in the area, I made some other cosmetic improvements:
- Applying strings::Substitute() to perforated LOG statements.
- Replacing 'OVERRIDE' with 'override'.
- Removing 'virtual' from overridden methods.
- Removing unnecessary std:: prefixes.
- Adding virtual destructors to {Table,Tablet}Visitor.
- A few other miscellaneous tweaks.
The new virtual destructors caused sys_catalog-test to fail in a bizarre
way: the catalog manager ran ~TabletLoader defined in sys_catalog-test.cc
rather than the implicit one from catalog_manager.cc. I fixed this by
renaming the sys_catalog-test {Table,Tablet}Loader classes; not quite sure
why there was no collision at link-time in the first place.
Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.h
3 files changed, 204 insertions(+), 219 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/7826/4
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
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/7826
to look at the new patch set (#3).
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
catalog_manager: don't log deleted tables/tablets at startup
On long-lived clusters this list can get quite long.
While I was in the area, I made some other cosmetic improvements:
- Applying strings::Substitute() to perforated LOG statements.
- Replacing 'OVERRIDE' with 'override'.
- Removing 'virtual' from overridden methods.
- Removing unnecessary std:: prefixes.
- Adding virtual destructors to {Table,Tablet}Visitor.
- A few other miscellaneous tweaks.
The new virtual destructors caused sys_catalog-test to fail in a bizarre
way: the catalog manager ran ~TabletLoader defined in sys_catalog-test.cc
rather than the implicit one from catalog_manager.cc. I fixed this by
renaming the sys_catalog-test {Table,Tablet}Loader classes; not quite sure
why there was no collision at link-time in the first place.
Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.h
3 files changed, 204 insertions(+), 219 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/7826/3
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: catalog_manager: don't log deleted tables/tablets at startup
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7826/2//COMMIT_MSG
Commit Message:
PS2, Line 14: overriden
nit: overridden
http://gerrit.cloudera.org:8080/#/c/7826/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS2, Line 359: VLOG(2)
interesting: for tables the debug log level is 1. Is this intentional to have debug log level 2 for tablets?
PS2, Line 2616: WARNING
Maybe, it's worth set to up ERROR since this is an error condition and the control flow goes out of this function?
PS2, Line 2951: ...
nit: drop the ellipsis?
PS2, Line 2947: if (delay_millis <= 0) {
: LOG(WARNING) << "Request timed out: " << description();
: MarkFailed();
: } else {
: LOG(INFO) << Substitute("Scheduling retry of $0 with a delay of $1 ms (attempt = $2)...",
: description(), delay_millis, attempt_);
: master_->messenger()->ScheduleOnReactor(
: boost::bind(&RetryingTSRpcTask::RunDelayedTask, this, _1),
: MonoDelta::FromMilliseconds(delay_millis));
: return true;
: }
: return false;
nit: while you are at it, consider removing the extra-scope for better readability:
if (delay_millis <=0) {
...
return false;
}
...
return true;
--
To view, visit http://gerrit.cloudera.org:8080/7826
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes