Replace CHECK() with CHECK_*() so that when a check is failing, more … by nwangtw · Pull Request #2847 · apache/incubator-heron

nwangtw · 2018-04-03T23:50:02Z

so that when a check is failing, more useful information is logged.

This PR affects only the CHECK() calls in cpp files.

…useful information is logged

Yaliang

Since you are looking into those logging messages to be informative. Would you update each check with a descriptive message?

CHECK_EQ(a, b) << ": some crazy thing is happening"

Also give a before|after example will also be useful for reviews

nwangtw · 2018-04-04T17:36:51Z

Yeah. It is a good point.

I was changing the safe ones only in this PR to be quick. For the CHECK_EQ() << "...." case I need to do some tests to verify the behavior of the library. Let me see if I can find some time to run the tests.

nwangtw · 2018-04-04T18:39:49Z

Just did a test. Seems the checks work with << as expected.

Run 1:
int a = 1, b = 2;
CHECK(a > b) << "Failing check 'a > b'";

The output is:
Check failed: a > b Failing check 'a > b'

Run 2:
int a = 1, b = 2;
CHECK_GT(a, b) << "Failing check 'gt(a,b)'";

The output is:
Check failed: a > b (1 vs. 2) Failing check 'gt(a, b)'

Let me update the checks with << operator then.

Yaliang · 2018-04-04T18:44:40Z

Just pass you a document for glog: http://rpg.ifi.uzh.ch/docs/glog.html

nwangtw · 2018-04-04T18:44:49Z

And I will see what description I can add now.

nwangtw · 2018-04-04T18:45:26Z

Cool. Thx. @Yaliang

nwangtw · 2018-04-05T03:05:58Z

Updated descriptions

Yaliang

There is a logic change in the code, some incorrect message I believe is caused copying between lines?

In addition, I would prefer the way like Failed to find xxxx instead of xxx is not found in xxx list. Check other log messages, it favors a way like (I) did something or (I'm) doing something.

Yaliang · 2018-04-05T23:24:33Z

heron/common/src/cpp/network/baseconnection.cpp

+  CHECK(mState == INIT || mState == DISCONNECTED)
+      << "Deleting connection object while it is still connected";
  disableRateLimit();  // To free the config object
+  bufferevent_free(buffer_);


I think it's either unrelated change or need to be rebased

Yeah. It is an unrelated change and has been merged in a different PR.

Yaliang · 2018-04-05T23:28:44Z

heron/common/src/cpp/network/client.h

      return;
    }
-    CHECK(m->IsInitialized());
+    CHECK(m->IsInitialized()) << "Protobuf not initialized";


Isn't m represents a Message ?

Yaliang · 2018-04-05T23:30:26Z

heron/common/src/cpp/zookeeper/zkclient.cpp

      hostportlist_(hostportlist),
      client_global_watcher_cb_(std::move(global_watcher_cb)) {
-  CHECK(client_global_watcher_cb_);
+  CHECK(client_global_watcher_cb_) << "Client global watcher is not provided";


I think it's better to explicitly say it's a callback

Yaliang · 2018-04-05T23:32:00Z

heron/instance/src/cpp/boltimpl/bolt-instance.cpp

 void BoltInstance::Deactivate() {
  LOG(INFO) << "Not doing anything in Bolt Dacctivate";
-  CHECK(active_);
+  CHECK(!active_) << "Bolt instance is not active";


logic changed:

CHECK(active_) << "Bolt instance is not active"

Yaliang · 2018-04-05T23:37:05Z

heron/statemgrs/src/cpp/statemgr/heron-zkstatemgr.cpp

-                                              VCallback<> watcher) {
-  CHECK(watcher);
-  CHECK(!topology_name.empty());
+                                                   VCallback<> watcher) {


unrelated indent change

Yaliang · 2018-04-05T23:55:11Z

heron/stmgr/src/cpp/util/xor-manager.cpp


 void XorManager::create(sp_int32 _task_id, sp_int64 _key, sp_int64 _value) {
-  CHECK(tasks_.find(_task_id) != tasks_.end());
+  CHECK(tasks_.find(_task_id) != tasks_.end()) << "Task " << _task_id << " is not found";


I would prefer Failed to find Task xxx

Yaliang · 2018-04-05T23:57:22Z

heron/stmgr/src/cpp/manager/stmgr.cpp

  LOG(INFO) << "Received StartProcessing message from tmaster for "
            << _checkpoint_id;
-  CHECK(stateful_restorer_);
+  CHECK(stateful_restorer_) << "Stateful restorer doesn't exist when starting process";


maybe starting stateful process

Yaliang · 2018-04-05T23:57:59Z

heron/stmgr/src/cpp/manager/stmgr.cpp


 void StMgr::CreateTMasterClient(proto::tmaster::TMasterLocation* tmasterLocation) {
-  CHECK(!tmaster_client_);
+  CHECK(!tmaster_client_) << "TMaster client has already existed";


TMaster Client? and also change next line Tmaster Client to TMaster Client?

Yaliang · 2018-04-05T23:58:35Z

heron/stmgr/src/cpp/manager/stmgr.cpp


 void StMgr::StartStmgrServer() {
-  CHECK(!stmgr_server_);
+  CHECK(!stmgr_server_) << "Stmgr server is already running";


StmgrServer is already running? to keep the same way as the next line.

Yaliang · 2018-04-06T00:02:16Z

heron/stmgr/src/cpp/manager/stmgr-clientmgr.cpp

                                            const proto::system::HeronTupleSet2& _msg) {
  auto iter = clients_.find(_stmgr_id);
-  CHECK(iter != clients_.end());
+  CHECK(iter != clients_.end()) << "Stmgr " << _stmgr_id << " is not found in client list";


Would prefer Failed to find the client to Stmgr xxx

nwangtw · 2018-04-18T06:31:49Z

Thanks for reviewing.

This was a super quick PR and incremental improvement. It seems to be more time consuming now. I will leave this PR here for now and come back when I have the time.

Replace CHECK() with CHECK_*() so that when a check is failing, more …

d325381

…useful information is logged

Yaliang reviewed Apr 4, 2018

View reviewed changes

add check messages

c1ecdf6

Yaliang suggested changes Apr 6, 2018

View reviewed changes

Conversation

nwangtw commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yaliang left a comment

Choose a reason for hiding this comment

Uh oh!

nwangtw commented Apr 4, 2018

Uh oh!

nwangtw commented Apr 4, 2018

Uh oh!

Yaliang commented Apr 4, 2018

Uh oh!

nwangtw commented Apr 4, 2018

Uh oh!

nwangtw commented Apr 4, 2018

Uh oh!

nwangtw commented Apr 5, 2018

Uh oh!

Yaliang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwangtw commented Apr 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nwangtw commented Apr 3, 2018 •

edited

Loading