Copilot commented on code in PR #2233:
URL:
https://github.com/apache/incubator-pegasus/pull/2233#discussion_r2742609797
##########
src/client_lib/pegasus_client_factory_impl.h:
##########
@@ -32,11 +44,43 @@ class pegasus_client;
namespace client {
class pegasus_client_impl;
+/**
+ * @brief Implementation of Pegasus client factory.
+ *
+ * This class manages the lifecycle of Pegasus client instances and provides
+ * a centralized way to create and access client objects.
+ *
+ * Typical usage:
+ * @code
+ * auto *client = pegasus::client::pegasus_client_factory::create_client(
+ * "cluster", "table", "config.ini");
Review Comment:
This example calls `pegasus_client_factory::create_client(...)`, but the
factory API provides `initialize(...)` and `get_client(...)` (see
`src/include/pegasus/client.h:1194+`). Please update the snippet to reflect the
real API so generated docs aren’t misleading.
```suggestion
* pegasus::client::pegasus_client_factory::initialize("config.ini");
* auto *client =
pegasus::client::pegasus_client_factory::get_client("cluster", "table");
```
##########
src/client_lib/pegasus_client_impl.h:
##########
@@ -361,24 +898,29 @@ class pegasus_client_impl : public pegasus_client
}
};
-private:
- std::string _cluster_name;
- std::string _app_name;
- ::dsn::host_port _meta_server;
- ::dsn::apps::rrdb_client *_client;
-
- ///
- /// \brief _client_error_to_string
- /// store int to string for client call get_error_string()
- ///
+ private:
+ std::string _cluster_name;
+ std::string _app_name;
+ ::dsn::host_port _meta_server;
+ ::dsn::apps::rrdb_client *_client;
Review Comment:
There’s a redundant, differently-indented `private:` here even though the
class already entered `private:` at line 873. Consider removing this extra
access specifier or aligning it to the file’s established style (access
specifiers at column 0).
##########
src/client_lib/pegasus_client_impl.h:
##########
@@ -45,53 +58,256 @@ class task_tracker;
namespace pegasus {
namespace client {
+// Forward declarations
+struct internal_info;
+struct check_and_set_results;
+struct check_and_mutate_results;
+class pegasus_scanner;
+class abstract_pegasus_scanner;
+
+/**
+ * @brief Callback for async set operations.
+ * @param err Operation result code (0 for success).
+ * @param hashkey Hash key of the request.
+ * @param sortkey Sort key of the request.
+ * @param value Value passed to set.
+ */
+typedef std::function<void(int, std::string &&, std::string &&, std::string
&&)> async_set_callback_t;
Review Comment:
This callback typedef (and the similar ones below) doesn’t match the actual
async callback types used by the public API (`pegasus::pegasus_client` in
`src/include/pegasus/client.h`). As-is, it will generate misleading Doxygen and
introduces incompatible similarly-named types in `pegasus::client`. Suggest
removing these typedefs and documenting/using
`pegasus_client::async_*_callback_t` instead (or make the signatures exactly
match).
##########
src/client_lib/pegasus_client_impl.h:
##########
@@ -334,10 +854,27 @@ class pegasus_client_impl : public pegasus_client
static const ::dsn::blob _max;
};
+ /**
+ * @brief Convert server error code to client error code
+ *
+ * @param server_error Server-side error code
+ * @return int Corresponding client error code
+ */
static int get_client_error(int server_error);
+
+ /**
+ * @brief Convert RocksDB error code to Pegasus server error code
+ *
+ * @param rocskdb_error RocksDB error code
+ * @return int Corresponding Pegasus server error code
+ */
static int get_rocksdb_server_error(int rocskdb_error);
Review Comment:
Typo in parameter name/documentation: `rocskdb_error` should be
`rocksdb_error`. Since parameter names aren’t part of the function type,
consider renaming it consistently in both the header and
`src/client_lib/pegasus_client_impl.cpp:1328` for readability, and fix the
`@param` tag accordingly.
```suggestion
* @param rocksdb_error RocksDB error code
* @return int Corresponding Pegasus server error code
*/
static int get_rocksdb_server_error(int rocksdb_error);
```
##########
src/client_lib/pegasus_client_impl.h:
##########
@@ -225,67 +626,186 @@ class pegasus_client_impl : public pegasus_client
async_check_and_mutate_callback_t
&&callback = nullptr,
int timeout_milliseconds = 5000)
override;
+ /**
+ * @brief Get time-to-live (TTL) for a key-value pair
+ *
+ * @param hashkey The hash key
+ * @param sortkey The sort key
+ * @param ttl_seconds Output parameter for TTL in seconds (-1 for no TTL)
+ * @param timeout_milliseconds Operation timeout in milliseconds (default
5000)
+ * @param info Optional internal info output
+ * @return int Operation result code (0 for success)
+ */
virtual int ttl(const std::string &hashkey,
const std::string &sortkey,
int &ttl_seconds,
int timeout_milliseconds = 5000,
internal_info *info = nullptr) override;
+ /**
+ * @brief Get a scanner for range query
+ *
+ * @param hashkey The hash key to scan
+ * @param start_sortkey Start sort key of the range (inclusive)
+ * @param stop_sortkey Stop sort key of the range (exclusive)
+ * @param options Scan options like sort direction
+ * @param scanner Output parameter for the created scanner
+ * @return int Operation result code (0 for success)
+ */
virtual int get_scanner(const std::string &hashkey,
const std::string &start_sortkey,
const std::string &stop_sortkey,
const scan_options &options,
pegasus_scanner *&scanner) override;
+ /**
+ * @brief Asynchronously get a scanner for range query
+ *
+ * @param hashkey The hash key to scan
+ * @param start_sortkey Start sort key of the range (inclusive)
+ * @param stop_sortkey Stop sort key of the range (exclusive)
+ * @param options Scan options like sort direction
+ * @param callback Callback function to handle the async result
+ */
virtual void async_get_scanner(const std::string &hashkey,
const std::string &start_sortkey,
const std::string &stop_sortkey,
const scan_options &options,
async_get_scanner_callback_t &&callback)
override;
+ /**
+ * @brief Get multiple scanners for parallel scanning across partitions
+ *
+ * @param max_split_count Maximum number of scanners to create (partitions
to scan)
+ * @param options Scan options like sort direction
+ * @param scanners Output vector for the created scanners
+ * @return int Operation result code (0 for success)
+ */
virtual int get_unordered_scanners(int max_split_count,
const scan_options &options,
std::vector<pegasus_scanner *>
&scanners) override;
+ /**
+ * @brief Asynchronously get multiple scanners for parallel scanning
+ *
+ * @param max_split_count Maximum number of scanners to create (partitions
to scan)
+ * @param options Scan options like sort direction
+ * @param callback Callback function to handle the async result
+ */
virtual void
async_get_unordered_scanners(int max_split_count,
const scan_options &options,
async_get_unordered_scanners_callback_t
&&callback) override;
- /// \internal
- /// This is an internal function for duplication.
- /// \see pegasus::server::pegasus_mutation_duplicator
+ /**
+ * @internal
+ * @brief Internal method for data duplication
+ * @param rpc Duplication RPC request
+ * @param callback Callback function for async result
+ * @param tracker Task tracker for managing async operations
+ * @see pegasus::server::pegasus_mutation_duplicator
+ */
void async_duplicate(dsn::apps::duplicate_rpc rpc,
std::function<void(dsn::error_code)> &&callback,
dsn::task_tracker *tracker);
+ /**
+ * @brief Get error description string for error code
+ *
+ * @param error_code The error code to lookup
+ * @return const char* Description of the error
+ */
virtual const char *get_error_string(int error_code) const override;
+ /**
+ * @brief Initialize error code mappings
+ *
+ * This method initializes the mapping between server error codes
+ * and client error codes, and should be called once during startup.
+ */
static void init_error();
+ /**
+ * @brief Implementation of Pegasus scanner for range queries
+ *
+ * This class provides the concrete implementation for scanning key ranges
in Pegasus.
+ * It supports both synchronous and asynchronous scanning operations, with
options
+ * for full scans or partial range scans.
+ */
class pegasus_scanner_impl : public pegasus_scanner
{
public:
+ /**
+ * @brief Get next key-value pair from scanner
+ * @param hashkey Output parameter for hash key
+ * @param sortkey Output parameter for sort key
+ * @param value Output parameter for value
+ * @param info Optional internal info output
+ * @return int Operation result code (0 for success)
+ */
int next(std::string &hashkey,
std::string &sortkey,
std::string &value,
internal_info *info = nullptr) override;
+ /**
+ * @brief Get count of remaining items in current batch
+ *
+ * @param count Output parameter for item count
+ * @param info Optional internal info output
+ * @return int Operation result code (0 for success)
+ */
int next(int32_t &count, internal_info *info = nullptr) override;
- void async_next(async_scan_next_callback_t &&) override;
+ /**
+ * @brief Asynchronously get next key-value pair from scanner.
+ * @param callback Callback function to handle the async result
+ * The callback parameters are:
+ * - error_code: Operation result code (0 for success)
+ * - hashkey: The hash key of the scanned item
+ * - sortkey: The sort key of the scanned item
+ * - value: The value of the scanned item
Review Comment:
The callback parameter list described here is incomplete for
`async_scan_next_callback_t` in the real API
(`src/include/pegasus/client.h:316-323` also includes `internal_info`,
`expire_ts_seconds`, and `kv_count`). Please update this doc (or reference the
typedef) to match the actual callback contract.
```suggestion
* @param callback Callback function to handle the async result.
*
* The exact callback signature is defined by
::pegasus::client::async_scan_next_callback_t
* in pegasus/client.h. The callback parameters include:
* - error_code: Operation result code (0 for success)
* - hashkey: The hash key of the scanned item
* - sortkey: The sort key of the scanned item
* - value: The value of the scanned item
* - internal_info: Optional internal information about the
scanned item
* - expire_ts_seconds: Expiration timestamp (in seconds) of the
scanned item
* - kv_count: Number of key-value pairs remaining in the current
batch
```
##########
src/client_lib/doxygen/README.md:
##########
@@ -0,0 +1,66 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+# Pegasus C++ Client Doxygen Guide
+
+This document explains how to install Doxygen and generate the Pegasus C++
client API
+documentation on **macOS** and **Linux**.
+
+## Directory Layout
+
+- `docs/doxygen/Doxyfile`: Preconfigured Doxygen configuration
+- `docs/doxygen/output/html`: Generated HTML output
+
+> If you update the C++ client headers or comments, re-run the generation
command to
+> refresh the documentation.
+
+## 1. Install Doxygen
+
+### macOS (Homebrew)
+
+```bash
+brew install doxygen
+```
+
+### Linux (Debian / Ubuntu)
+
+```bash
+sudo apt-get update
+sudo apt-get install -y doxygen
+```
+
+Verify the installation:
+
+```bash
+doxygen -v
+```
+
+## 2. Generate Documentation
+
+Run this from the repository root:
+
+```bash
+doxygen docs/doxygen/Doxyfile
+```
Review Comment:
This command/path doesn’t match the repository layout: the Doxyfile added by
this PR is `src/client_lib/doxygen/Doxyfile`, not `docs/doxygen/Doxyfile`.
Please fix the command (and the output path below) accordingly.
##########
src/client_lib/doxygen/README.md:
##########
@@ -0,0 +1,66 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+# Pegasus C++ Client Doxygen Guide
+
+This document explains how to install Doxygen and generate the Pegasus C++
client API
+documentation on **macOS** and **Linux**.
+
+## Directory Layout
+
+- `docs/doxygen/Doxyfile`: Preconfigured Doxygen configuration
+- `docs/doxygen/output/html`: Generated HTML output
+
Review Comment:
The paths in this layout don’t match the files added by this PR: the Doxygen
config lives under `src/client_lib/doxygen/`, not `docs/doxygen/`. Please
update these references (or move the config under `docs/`) so the guide is
runnable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
