Get rid of Arrow test utils

[suquark]

This PR removes arrow test utils because some of its macros conflict with GLOG and other libraries. This will also provide unified stack traces.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10723/
Test FAILed.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10724/
Test FAILed.

[pcmoritz]

The logging improvements look good.

For the python client, does it make much sense to put much time into it? We want to do #3541 instead, right?

[suquark]

@pcmoritz That makes sense. I also observed some double frees in the python wrapper (properly related to unique pointer), so I have reverted those changes.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10730/
Test FAILed.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10736/
Test PASSed.

[pcmoritz]

@guoyuhong Please have a look at the logging_provider change, does that look good to you? It undoes some of the changes you did in #2816, are they still needed? If yes, we should document it in the source code.

[guoyuhong]

@suquark @pcmoritz One concern is that we expose the macro RAY_USE_GLOG in logging.h. If we only use this in raylet, this change is OK. If libraylet.a this is used in other project as a lib, the user needs to also define the macro RAY_USE_GLOG to use this lib correctly. I found this issue when I try to do similar change in Arrow's logging lib which is a third-party lib in ray.

[guoyuhong]

It is fine to move class CerrLog to logging.h. Shall we still hide the GLOG related implementation into logging.cc?

[suquark]

The problems I found is macros like DCHECK are exported in the Arrow's logging.h. They conflicts with GLOG's DCHECK. I have undefined them in id.h.
Even if we tried to hide GLOG, the libraries depending on Ray will still affected by Arrow's logging. I think a better approach is to rename macros in Arrow. C++ macro is a terrible design in this case :(

[guoyuhong]

@suquark Shall we consider add RAY_ prefix to make the macro RAY_DCHECK.

[pcmoritz]

@guoyuhong Another solution (since we don't have control over arrow's DCHECK) is to remove logging.h in the plasma/common.h file. I'm happy to create a patch about that for arrow, so we can merge that first and then merge this PR.

[pcmoritz]

apache/arrow#3392

[guoyuhong]

@pcmoritz That is better~

[suquark]

I have rebased this branch to use the new Arrow version.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10842/
Test FAILed.

[pcmoritz]

@suquark: There are still some clashes, but we can resolve them in this PR without further changes to arrow. I'd suggest we get rid of #include "arrow/util/logging.h" everywhere and define our own macro in ray/util/logging.h, which is RAY_ARROW_CHECK_OK, which checks that arrow::Status is ok and bails out with glog if it is not. This will also provide unified stack traces.

[pcmoritz]

@suquark Do you want to make these changes so we can get this merged?

[suquark]

@pcmoritz Sorry for my late response. Let me try to make these changes.

[guoyuhong]

I still recommend to hide the following typedef to logging.cc instead of logging.h. And in logging.h we may use void * to represent the provider.

#ifdef RAY_USE_GLOG
#include "glog/logging.h"
typedef google::LogMessage LoggingProvider;
#else
typedef ray::CerrLog LoggingProvider;
#endif

Only in this case, the raylet lib user does not to define RAY_USE_GLOG in there own compiling env. Actually, we have streaming code as a separate module which use raylet lib. This will encapsule the logging implementation.

[guoyuhong]

LGTM. Thanks for your efforts!

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10909/
Test PASSed.

[pcmoritz]

There is a little bit of linting left:

diff --git a/src/ray/object_manager/test/object_manager_stress_test.cc b/src/ray/object_manager/test/object_manager_stress_test.cc
index 8b5d715..3f2d50e 100644
--- a/src/ray/object_manager/test/object_manager_stress_test.cc
+++ b/src/ray/object_manager/test/object_manager_stress_test.cc
@@ -180,7 +180,7 @@ class TestObjectManagerBase : public ::testing::Test {
     int64_t metadata_size = sizeof(metadata);
     std::shared_ptr<Buffer> data;
     RAY_ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata,
-                                 metadata_size, &data));
+                                     metadata_size, &data));
     RAY_ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id()));
     return object_id;
   }
diff --git a/src/ray/object_manager/test/object_manager_test.cc b/src/ray/object_manager/test/object_manager_test.cc
index e47c218..a71d763 100644
--- a/src/ray/object_manager/test/object_manager_test.cc
+++ b/src/ray/object_manager/test/object_manager_test.cc
@@ -169,7 +169,7 @@ class TestObjectManagerBase : public ::testing::Test {
     int64_t metadata_size = sizeof(metadata);
     std::shared_ptr<Buffer> data;
     RAY_ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata,
-                                 metadata_size, &data));
+                                     metadata_size, &data));
     RAY_ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id()));
     return object_id;
   }
diff --git a/src/ray/raylet/object_manager_integration_test.cc b/src/ray/raylet/object_manager_integration_test.cc
index a6ac6f3..8610205 100644
--- a/src/ray/raylet/object_manager_integration_test.cc
+++ b/src/ray/raylet/object_manager_integration_test.cc
@@ -105,7 +105,7 @@ class TestObjectManagerBase : public ::testing::Test {
     int64_t metadata_size = sizeof(metadata);
     std::shared_ptr<Buffer> data;
     RAY_ARROW_CHECK_OK(client.Create(object_id.to_plasma_id(), data_size, metadata,
-                                 metadata_size, &data));
+                                     metadata_size, &data));
     RAY_ARROW_CHECK_OK(client.Seal(object_id.to_plasma_id()));
     return object_id;
   }
diff --git a/src/ray/status.h b/src/ray/status.h
index 143ffc1..06ac584 100644
--- a/src/ray/status.h
+++ b/src/ray/status.h
@@ -45,9 +45,9 @@
 #define RAY_CHECK_OK(s) RAY_CHECK_OK_PREPEND(s, "Bad status")
 
 // This macro is used to replace the "ARROW_CHECK_OK_PREPEND" macro.
-#define RAY_ARROW_CHECK_OK_PREPEND(to_call, msg)                \
-  do {                                                      \
-    ::arrow::Status _s = (to_call);                         \
+#define RAY_ARROW_CHECK_OK_PREPEND(to_call, msg)          \
+  do {                                                    \
+    ::arrow::Status _s = (to_call);                       \
     RAY_CHECK(_s.ok()) << (msg) << ": " << _s.ToString(); \
   } while (false)

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10918/
Test PASSed.

[suquark]

@pcmoritz It's ready to merge. The test error is not related.