You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/03/17 23:00:54 UTC
[kudu-CR] [scanner] made code up-to-date with the TODO comment
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6416
Change subject: [scanner] made code up-to-date with the TODO comment
......................................................................
[scanner] made code up-to-date with the TODO comment
Replaced LOG(INFO) with VLOG(1) in the event of expiring a scanner.
Removed corresponding TODO comment.
Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
---
M src/kudu/tserver/scanners.cc
1 file changed, 15 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/6416/1
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6416/3/src/kudu/tserver/scanners.cc
File src/kudu/tserver/scanners.cc:
Line 30: #include "kudu/util/thread.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6416
to look at the new patch set (#4).
Change subject: [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
......................................................................
[scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Made code up-to-date with the TODO comment: replaced LOG(INFO) with
VLOG(1) in the event of scanner expiration.
Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
---
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
2 files changed, 32 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/6416/4
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6416/1/src/kudu/tserver/scanners.cc
File src/kudu/tserver/scanners.cc:
Line 169: VLOG(1) << "Expiring scanner id: " << it->first << ", of tablet " << scanner->tablet_id()
> Nit: since you're already modifying this, mind using strings::Substitute()?
Done
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6416/1/src/kudu/tserver/scanners.cc
File src/kudu/tserver/scanners.cc:
Line 169: VLOG(1) << "Expiring scanner id: " << it->first << ", of tablet " << scanner->tablet_id()
Nit: since you're already modifying this, mind using strings::Substitute()? Makes it easier to visualize the entirety of complex strings like this one.
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
......................................................................
Patch Set 1:
hrm, I'm not sure about this change. In the case that we get "scanner not found" errors on the client, it's useful to grep the logs and find when it expired, etc. Did you find a case where this was extremely noisy?
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
......................................................................
[scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Made code up-to-date with the TODO comment: replaced LOG(INFO) with
VLOG(1) in the event of scanner expiration.
Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Reviewed-on: http://gerrit.cloudera.org:8080/6416
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
2 files changed, 32 insertions(+), 22 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@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: Tidy Bot
[kudu-CR] [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#2).
Change subject: [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
......................................................................
[scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Made code up-to-date with the TODO comment: replaced LOG(INFO) with
VLOG(1) in the event of scanner expiration.
Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
---
M src/kudu/tserver/scanners.cc
1 file changed, 15 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/6416/2
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
......................................................................
Patch Set 5:
> hrm, I'm not sure about this change. In the case that we get
> "scanner not found" errors on the client, it's useful to grep the
> logs and find when it expired, etc. Did you find a case where this
> was extremely noisy?
I did this change since I saw that 'TODO' and the metric is already updated on scanner expiration. No, I didn't find it noisy.
Frankly, after some consideration I think it was rather silly to following that TODO: we recently saw that log message helped people to find out scanners were expiring.
I will send a patch to return LOG(INFO) back.
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6416
to look at the new patch set (#3).
Change subject: [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
......................................................................
[scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Made code up-to-date with the TODO comment: replaced LOG(INFO) with
VLOG(1) in the event of scanner expiration.
Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
---
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
2 files changed, 31 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/6416/3
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [scanner] LOG(INFO) --> VLOG(1) on scanner expiration
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6416
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I78f0b07131aaf4dc4a2cb72bed1ce1647a201b68
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@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: Tidy Bot
Gerrit-HasComments: No