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/05/10 23:57:14 UTC
[kudu-CR] KUDU-2005: actionable error messages from webserver
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6848
Change subject: KUDU-2005: actionable error messages from webserver
......................................................................
KUDU-2005: actionable error messages from webserver
As it turned out, squeasel outputs only errors via the log_message
callback. Let's output them with ERROR severity into Kudu log
(they were output as INFO messages prior to this patch).
Also, if the embedded squeasel webserver fails to start, add the last
error message from it (if any) into the RuntimeError status message
returned from Webserver::Start().
Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77
---
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver.h
3 files changed, 25 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6848/1
--
To view, visit http://gerrit.cloudera.org:8080/6848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-2005: actionable error messages from webserver
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2005: actionable error messages from webserver
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6848/1/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
PS1, Line 246: TryRunLsof(addr);
> Put this after the error message below, since the below puts it in context.
I'm not sure I understand -- the error message below is generated from squeasel error message and the http_address_. TryRunLsof() just outputs its messages into the log, since the second argument of TryRunLsof() is empty. Also, that output goes into the WARNING log file, not ERROR one.
I looked into the squeasel source code and I could not find the way to detect that condition if not parsing the error message returned from it.
That's the excerpt:
cry(fc(ctx), "%s: cannot bind to %.*s: %d (%s)", __func__,
(int) vec.len, vec.ptr, ERRNO, strerror(errno));
So, I would rather leave it as is for now since I'm not a fan of parsing error messages. Expecting/checking for particular error message patterns in tests is more or less tolerable, but not here. There are too many risks: the squeasel code can change, that particular error message might be not the last one we receive, etc.
Let's change the output from TryRunLsof() to remove the ' "Failed to bind to " << addr.ToString() << ". "' part -- that would be less confusion to the readers (I will post that patch separately).
--
To view, visit http://gerrit.cloudera.org:8080/6848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77
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: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2005: actionable error messages from webserver
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: KUDU-2005: actionable error messages from webserver
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6848/1/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
PS1, Line 246: TryRunLsof(addr);
Put this after the error message below, since the below puts it in context.
It'd also be nice to do this lsof step only when we get an address-in-use error, or have good reason to suspect that is the error, rather than for any failure.
--
To view, visit http://gerrit.cloudera.org:8080/6848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2005: actionable error messages from webserver
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: KUDU-2005: actionable error messages from webserver
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6848/1/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:
PS1, Line 246: TryRunLsof(addr);
> I'm not sure I understand -- the error message below is generated from sque
Thanks for looking into the squeasel code to see if we could do better re: lsof check. I agree it's best not to parse the message.
--
To view, visit http://gerrit.cloudera.org:8080/6848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77
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: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2005: actionable error messages from webserver
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: KUDU-2005: actionable error messages from webserver
......................................................................
KUDU-2005: actionable error messages from webserver
As it turned out, squeasel outputs only errors via the log_message
callback. Let's output them with ERROR severity into Kudu log
(they were output as INFO messages prior to this patch).
Also, if the embedded squeasel webserver fails to start, add the last
error message from it (if any) into the RuntimeError status message
returned from Webserver::Start().
Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77
Reviewed-on: http://gerrit.cloudera.org:8080/6848
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>
---
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver.h
3 files changed, 25 insertions(+), 6 deletions(-)
Approvals:
Will Berkeley: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6848
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia2315f7ee88c8835a36e5174cb25132967429a77
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: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>