[core] Cover cpplint for src/ray/pubsub (#50732)

## Why are these changes needed? As part of the initiative to introduce cpplint into the pre-commit hook, we are gradually cleaning up C++ folders to ensure compliance with code style requirements. This issue focuses on cleaning up `src/ray/pubsub`.  - This is the command that I have ran ``` cpplint \ --filter=-whitespace/line_length,\ -build/c++11,\ -build/c++14,\ -build/c++17,\ -readability/braces,\ -whitespace/indent_namespace,\ -runtime/int,\ -runtime/references,\ -build/include_order \ src/ray/pubsub/*.h \ src/ray/pubsub/*.cc \ src/ray/pubsub/**/*.h \ src/ray/pubsub/**/*.cc ``` - The log output ``` Skipping input 'src/ray/pubsub/**/*.h': Can't open for reading src/ray/pubsub/publisher.cc:416: Add #include <memory> for make_unique<> [build/include_what_you_use] [4] src/ray/pubsub/publisher.cc:442: Add #include <utility> for move [build/include_what_you_use] [4] src/ray/pubsub/publisher.cc:504: Add #include <vector> for vector<> [build/include_what_you_use] [4] src/ray/pubsub/publisher.cc:543: Add #include <string> for string [build/include_what_you_use] [4] Done processing src/ray/pubsub/publisher.cc src/ray/pubsub/publisher.h:94: Single-parameter constructors should be marked explicit. [runtime/explicit] [4] src/ray/pubsub/publisher.h:309: Add #include <vector> for vector<> [build/include_what_you_use] [4] src/ray/pubsub/publisher.h:316: Add #include <utility> for move [build/include_what_you_use] [4] src/ray/pubsub/publisher.h:454: Add #include <memory> for unique_ptr<> [build/include_what_you_use] [4] Done processing src/ray/pubsub/publisher.h src/ray/pubsub/subscriber.cc:319: Add #include <memory> for make_unique<> [build/include_what_you_use] [4] src/ray/pubsub/subscriber.cc:461: Add #include <vector> for vector<> [build/include_what_you_use] [4] src/ray/pubsub/subscriber.cc:482: Add #include <utility> for move [build/include_what_you_use] [4] src/ray/pubsub/subscriber.cc:522: Add #include <string> for string [build/include_what_you_use] [4] Done processing src/ray/pubsub/subscriber.cc src/ray/pubsub/subscriber.h:332: Add #include <vector> for vector<> [build/include_what_you_use] [4] src/ray/pubsub/subscriber.h:413: Add #include <string> for string [build/include_what_you_use] [4] src/ray/pubsub/subscriber.h:492: Add #include <memory> for unique_ptr<> [build/include_what_you_use] [4] src/ray/pubsub/subscriber.h:497: Add #include <utility> for pair<> [build/include_what_you_use] [4] Done processing src/ray/pubsub/subscriber.h src/ray/pubsub/test/integration_test.cc:173: Add #include <utility> for move [build/include_what_you_use] [4] src/ray/pubsub/test/integration_test.cc:234: Add #include <vector> for vector<> [build/include_what_you_use] [4] Done processing src/ray/pubsub/test/integration_test.cc src/ray/pubsub/test/publisher_test.cc:30: Do not use namespace using-directives. Use using-declarations instead. [build/namespaces] [5] src/ray/pubsub/test/publisher_test.cc:613: Add #include <memory> for make_shared<> [build/include_what_you_use] [4] src/ray/pubsub/test/publisher_test.cc:827: Add #include <algorithm> for max [build/include_what_you_use] [4] src/ray/pubsub/test/publisher_test.cc:1063: Add #include <vector> for vector<> [build/include_what_you_use] [4] src/ray/pubsub/test/publisher_test.cc:1248: Add #include <string> for string [build/include_what_you_use] [4] Done processing src/ray/pubsub/test/publisher_test.cc src/ray/pubsub/test/subscriber_test.cc:23: Extra space before [ [whitespace/braces] [5] src/ray/pubsub/test/subscriber_test.cc:239: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] src/ray/pubsub/test/subscriber_test.cc:245: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] src/ray/pubsub/test/subscriber_test.cc:282: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] src/ray/pubsub/test/subscriber_test.cc:292: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] src/ray/pubsub/test/subscriber_test.cc:298: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] src/ray/pubsub/test/subscriber_test.cc:299: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] src/ray/pubsub/test/subscriber_test.cc:328: Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b) [readability/check] [2] src/ray/pubsub/test/subscriber_test.cc:379: Consider using ASSERT_GT instead of ASSERT_TRUE(a > b) [readability/check] [2] src/ray/pubsub/test/subscriber_test.cc:410: Consider using ASSERT_GT instead of ASSERT_TRUE(a > b) [readability/check] [2] src/ray/pubsub/test/subscriber_test.cc:418: Consider using ASSERT_GT instead of ASSERT_TRUE(a > b) [readability/check] [2] src/ray/pubsub/test/subscriber_test.cc:108: Add #include <utility> for move [build/include_what_you_use] [4] src/ray/pubsub/test/subscriber_test.cc:118: Add #include <deque> for deque<> [build/include_what_you_use] [4] src/ray/pubsub/test/subscriber_test.cc:119: Add #include <queue> for queue<> [build/include_what_you_use] [4] src/ray/pubsub/test/subscriber_test.cc:208: Add #include <memory> for shared_ptr<> [build/include_what_you_use] [4] src/ray/pubsub/test/subscriber_test.cc:209: Add #include <unordered_map> for unordered_map<> [build/include_what_you_use] [4] src/ray/pubsub/test/subscriber_test.cc:210: Add #include <unordered_set> for unordered_set<> [build/include_what_you_use] [4] src/ray/pubsub/test/subscriber_test.cc:909: Add #include <string> for string [build/include_what_you_use] [4] src/ray/pubsub/test/subscriber_test.cc:930: Add #include <vector> for vector<> [build/include_what_you_use] [4] Done processing src/ray/pubsub/test/subscriber_test.cc Total errors found: 42 ``` > I've separated the changes for each cpplint error into separate commits. ## Related issue number  Closes #50728 ## Checks - [x] I've signed off every commit(by using the -s flag, i.e., `git commit -s`) in this PR. - [x] I've run `scripts/format.sh` to lint the changes in this PR. - [ ] I've included any doc changes needed for https://docs.ray.io/en/master/. - [ ] I've added any new APIs to the API Reference. For example, if I added a method in Tune, I've added it in `doc/source/tune/api/` under the corresponding `.rst` file. - [x] I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/ - Testing Strategy - [ ] Unit tests - [ ] Release tests - [ ] This PR is not tested :( --------- Signed-off-by: Cheyu Wu <[email protected]>
ray-project · Feb 23, 2025 · 5db1b9a · 5db1b9a
1 parent 5566c1a
commit 5db1b9a
Show file tree

Hide file tree

Showing 8 changed files with 48 additions and 14 deletions.
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -46,8 +46,8 @@ repos:
     rev: 2.0.0
     hooks:
       - id: cpplint
-        args: ["--filter=-whitespace/line_length,-build/c++11,-build/c++14,-build/c++17,-readability/braces,-whitespace/indent_namespace,-runtime/int,-runtime/references,-build/include_order"]
-        files: ^src/ray/(util|raylet_client|scheduling)/.*\.(h|cc)$
+        args: ["--filter=-whitespace/braces,-whitespace/line_length,-build/c++11,-build/c++14,-build/c++17,-readability/braces,-whitespace/indent_namespace,-runtime/int,-runtime/references,-build/include_order"]
+        files: ^src/ray/(util|raylet_client|scheduling|pubsub(?:/.*)?)/.*\.(h|cc)$
 
   - repo: https://github.com/psf/black
     rev: 22.10.0

diff --git a/src/ray/pubsub/publisher.cc b/src/ray/pubsub/publisher.cc
@@ -14,6 +14,11 @@
 
 #include "ray/pubsub/publisher.h"
 
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "ray/common/ray_config.h"
 
 namespace ray {

diff --git a/src/ray/pubsub/publisher.h b/src/ray/pubsub/publisher.h
@@ -18,9 +18,12 @@
 
 #include <deque>
 #include <functional>
+#include <memory>
 #include <queue>
 #include <string>
 #include <string_view>
+#include <utility>
+#include <vector>
 
 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"
@@ -91,7 +94,7 @@ class EntityState {
 /// Also supports subscribers to all keys in the channel.
 class SubscriptionIndex {
  public:
-  SubscriptionIndex(rpc::ChannelType channel_type);
+  explicit SubscriptionIndex(rpc::ChannelType channel_type);
   ~SubscriptionIndex() = default;
 
   SubscriptionIndex(SubscriptionIndex &&) noexcept = default;

diff --git a/src/ray/pubsub/subscriber.cc b/src/ray/pubsub/subscriber.cc
@@ -14,6 +14,11 @@
 
 #include "ray/pubsub/subscriber.h"
 
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
 namespace ray {
 
 namespace pubsub {

diff --git a/src/ray/pubsub/subscriber.h b/src/ray/pubsub/subscriber.h
@@ -17,7 +17,11 @@
 #include <grpcpp/grpcpp.h>
 #include <gtest/gtest_prod.h>
 
+#include <memory>
 #include <queue>
+#include <string>
+#include <utility>
+#include <vector>
 
 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"

diff --git a/src/ray/pubsub/test/integration_test.cc b/src/ray/pubsub/test/integration_test.cc
@@ -14,6 +14,8 @@
 
 #include <memory>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "absl/synchronization/blocking_counter.h"
 #include "absl/synchronization/mutex.h"

diff --git a/src/ray/pubsub/test/publisher_test.cc b/src/ray/pubsub/test/publisher_test.cc
@@ -14,6 +14,11 @@
 
 #include "ray/pubsub/publisher.h"
 
+#include <algorithm>
+#include <memory>
+#include <string>
+#include <vector>
+
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "ray/common/asio/instrumented_io_context.h"
@@ -27,7 +32,8 @@ namespace {
 const NodeID kDefaultPublisherId = NodeID::FromRandom();
 }
 
-using namespace pub_internal;
+using pub_internal::SubscriberState;
+using pub_internal::SubscriptionIndex;
 
 class PublisherTest : public ::testing::Test {
  public:

diff --git a/src/ray/pubsub/test/subscriber_test.cc b/src/ray/pubsub/test/subscriber_test.cc
@@ -14,6 +14,15 @@
 
 #include "ray/pubsub/subscriber.h"
 
+#include <deque>
+#include <memory>
+#include <queue>
+#include <string>
+#include <unordered_map>
+#include <unordered_set>
+#include <utility>
+#include <vector>
+
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "ray/common/asio/instrumented_io_context.h"
@@ -236,13 +245,13 @@ TEST_F(SubscriberTest, TestBasicSubscription) {
   ASSERT_TRUE(ReplyLongPolling(channel, objects_batched));
   // Make sure the long polling batch works as expected.
   for (const auto &object_id : objects_batched) {
-    ASSERT_TRUE(object_subscribed_[object_id] == 1);
+    ASSERT_EQ(object_subscribed_[object_id], 1);
   }
 
   // Publish the objects again, and subscriber should receive it.
   ASSERT_TRUE(ReplyLongPolling(channel, objects_batched));
   for (const auto &object_id : objects_batched) {
-    ASSERT_TRUE(object_subscribed_[object_id] == 2);
+    ASSERT_EQ(object_subscribed_[object_id], 2);
   }
 
   ASSERT_TRUE(subscriber_->Unsubscribe(channel, owner_addr, object_id.Binary()));
@@ -279,7 +288,7 @@ TEST_F(SubscriberTest, TestIgnoreOutofOrderMessage) {
   ASSERT_EQ(2, owner_client->GetReportedMaxProcessedSequenceId());
 
   for (const auto &object_id : objects_batched) {
-    ASSERT_TRUE(object_subscribed_[object_id] == 1);
+    ASSERT_EQ(object_subscribed_[object_id], 1);
   }
 
   // By resetting the sequence_id, the message now come out of order,
@@ -289,14 +298,14 @@ TEST_F(SubscriberTest, TestIgnoreOutofOrderMessage) {
 
   // Make sure the long polling batch works as expected.
   for (const auto &object_id : objects_batched) {
-    ASSERT_TRUE(object_subscribed_[object_id] == 1);
+    ASSERT_EQ(object_subscribed_[object_id], 1);
   }
 
   // message arrives out of order (sequence_id 4 comes before 3),
   // we will ignore message with sequence id 3.
   ASSERT_TRUE(ReplyLongPolling(channel, objects_batched, {4, 3}));
-  ASSERT_TRUE(object_subscribed_[object_id] == 2);
-  ASSERT_TRUE(object_subscribed_[object_id1] == 1);
+  ASSERT_EQ(object_subscribed_[object_id], 2);
+  ASSERT_EQ(object_subscribed_[object_id1], 1);
   ASSERT_EQ(4, owner_client->GetReportedMaxProcessedSequenceId());
 }
 
@@ -325,7 +334,7 @@ TEST_F(SubscriberTest, TestPublisherFailsOver) {
   ASSERT_EQ(2, owner_client->GetReportedMaxProcessedSequenceId());
 
   for (const auto &object_id : objects_batched) {
-    ASSERT_TRUE(object_subscribed_[object_id] == 1);
+    ASSERT_EQ(object_subscribed_[object_id], 1);
   }
 
   // By resetting the sequence_id, the message now come out of order,
@@ -376,7 +385,7 @@ TEST_F(SubscriberTest, TestSingleLongPollingWithMultipleSubscriptions) {
   // Make sure the long polling batch works as expected.
   for (const auto &object_id : objects_batched) {
     // RAY_LOG(ERROR) << "haha " << object_subscribed_[object_id];
-    ASSERT_TRUE(object_subscribed_[object_id] > 0);
+    ASSERT_GT(object_subscribed_[object_id], 0);
   }
 }
 
@@ -407,15 +416,15 @@ TEST_F(SubscriberTest, TestMultiLongPollingWithTheSameSubscription) {
   std::vector<ObjectID> objects_batched;
   objects_batched.push_back(object_id);
   ASSERT_TRUE(ReplyLongPolling(channel, objects_batched));
-  ASSERT_TRUE(object_subscribed_[object_id] > 0);
+  ASSERT_GT(object_subscribed_[object_id], 0);
   objects_batched.clear();
   object_subscribed_.clear();
 
   // New long polling should be made because the subscription is still alive.
   ASSERT_EQ(owner_client->GetNumberOfInFlightLongPollingRequests(), 1);
   objects_batched.push_back(object_id);
   ASSERT_TRUE(ReplyLongPolling(channel, objects_batched));
-  ASSERT_TRUE(object_subscribed_[object_id] > 0);
+  ASSERT_GT(object_subscribed_[object_id], 0);
 }
 
 TEST_F(SubscriberTest, TestCallbackNotInvokedForNonSubscribedObject) {