You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "empiredan (via GitHub)" <gi...@apache.org> on 2024/01/09 12:45:23 UTC
[PR] feat(http_client): add http_url class to help operate URL [incubator-pegasus]
empiredan opened a new pull request, #1843:
URL: https://github.com/apache/incubator-pegasus/pull/1843
(no comment)
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(http_client): add http_url class to help build URLs for http client [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1843:
URL: https://github.com/apache/incubator-pegasus/pull/1843#discussion_r1454708640
##########
src/http/test/http_client_test.cpp:
##########
@@ -32,10 +33,108 @@
#include "utils/error_code.h"
#include "utils/errors.h"
#include "utils/fmt_logging.h"
+#include "utils/strings.h"
#include "utils/test_macros.h"
namespace dsn {
+using http_url_case =
+ std::tuple<const char *, const char *, uint16_t, const char *, const char *, const char *>;
Review Comment:
Good idea, I've used `struct` for all of the cases that need many arguments.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(http_client): add http_url class to help build URLs for http client [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1843:
URL: https://github.com/apache/incubator-pegasus/pull/1843#discussion_r1454701939
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...) \
+ do { \
+ const auto code = (expr); \
+ if (dsn_unlikely(code != expected)) { \
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__), to_error_msg(code))); \
+ return dsn::error_s::make(to_error_code(code), msg); \
+ } \
+ } while (0)
+
+http_url::http_url() : _url(nullptr) {}
+
+http_url::~http_url() { free_curlu_object(); }
+
+http_url::http_url(http_url &&rhs) : _url(rhs._url) { rhs._url = nullptr; }
+
+http_url &http_url::operator=(http_url &&rhs)
+{
+ _url = rhs._url;
Review Comment:
OK, I've added for both copy and move assignment operators.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(http_client): add http_url class to help build URLs for http client [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1843:
URL: https://github.com/apache/incubator-pegasus/pull/1843#discussion_r1454706115
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...) \
+ do { \
+ const auto code = (expr); \
+ if (dsn_unlikely(code != expected)) { \
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__), to_error_msg(code))); \
+ return dsn::error_s::make(to_error_code(code), msg); \
+ } \
+ } while (0)
+
+http_url::http_url() : _url(nullptr) {}
+
+http_url::~http_url() { free_curlu_object(); }
+
+http_url::http_url(http_url &&rhs) : _url(rhs._url) { rhs._url = nullptr; }
+
+http_url &http_url::operator=(http_url &&rhs)
+{
+ _url = rhs._url;
Review Comment:
I've wrapped it with `std::unique_ptr` to avoid the problem, and add test cases for copy/move constructors, copy/move assignment operators.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(http_client): add http_url class to help build URLs for http client [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1843:
URL: https://github.com/apache/incubator-pegasus/pull/1843#discussion_r1454700886
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...) \
+ do { \
+ const auto code = (expr); \
+ if (dsn_unlikely(code != expected)) { \
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__), to_error_msg(code))); \
Review Comment:
`to_error_msg` will show underlying messages, thus we could keep it behind.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(http_client): add http_url class to help build URLs for http client [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1843:
URL: https://github.com/apache/incubator-pegasus/pull/1843#discussion_r1454706504
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...) \
+ do { \
+ const auto code = (expr); \
+ if (dsn_unlikely(code != expected)) { \
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__), to_error_msg(code))); \
+ return dsn::error_s::make(to_error_code(code), msg); \
+ } \
+ } while (0)
+
+http_url::http_url() : _url(nullptr) {}
+
+http_url::~http_url() { free_curlu_object(); }
+
+http_url::http_url(http_url &&rhs) : _url(rhs._url) { rhs._url = nullptr; }
+
+http_url &http_url::operator=(http_url &&rhs)
+{
+ _url = rhs._url;
+ rhs._url = nullptr;
+ return *this;
+}
+
+dsn::error_s http_url::init()
+{
+ if (_url != nullptr) {
+ free_curlu_object();
+ }
+
+ _url = curl_url();
+ if (_url == nullptr) {
+ return dsn::error_s::make(dsn::ERR_CURL_FAILED, "fail to initialize curl url");
+ }
Review Comment:
OK.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(http_client): add http_url class to help build URLs for http client [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1843:
URL: https://github.com/apache/incubator-pegasus/pull/1843#discussion_r1448336415
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...) \
+ do { \
+ const auto code = (expr); \
+ if (dsn_unlikely(code != expected)) { \
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__), to_error_msg(code))); \
+ return dsn::error_s::make(to_error_code(code), msg); \
+ } \
+ } while (0)
+
+http_url::http_url() : _url(nullptr) {}
+
+http_url::~http_url() { free_curlu_object(); }
+
+http_url::http_url(http_url &&rhs) : _url(rhs._url) { rhs._url = nullptr; }
+
+http_url &http_url::operator=(http_url &&rhs)
+{
+ _url = rhs._url;
Review Comment:
Prevent self assignment.
if (this != &rhs) {...}
##########
src/http/test/http_client_test.cpp:
##########
@@ -32,10 +33,108 @@
#include "utils/error_code.h"
#include "utils/errors.h"
#include "utils/fmt_logging.h"
+#include "utils/strings.h"
#include "utils/test_macros.h"
namespace dsn {
+using http_url_case =
+ std::tuple<const char *, const char *, uint16_t, const char *, const char *, const char *>;
Review Comment:
It's too long, how about adding a new struct which contains all the fields, the struct member names are self-explanatory, and it would be helpful to simplify the following code.
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...) \
+ do { \
+ const auto code = (expr); \
+ if (dsn_unlikely(code != expected)) { \
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__), to_error_msg(code))); \
Review Comment:
How about swap the places of error code and message, which is the behavior of other macros like LOG_AND_RETURN_NOT_TRUE, LOG_AND_RETURN_NOT_RDB_OK, etc.
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...) \
+ do { \
+ const auto code = (expr); \
+ if (dsn_unlikely(code != expected)) { \
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__), to_error_msg(code))); \
+ return dsn::error_s::make(to_error_code(code), msg); \
+ } \
+ } while (0)
+
+http_url::http_url() : _url(nullptr) {}
+
+http_url::~http_url() { free_curlu_object(); }
+
+http_url::http_url(http_url &&rhs) : _url(rhs._url) { rhs._url = nullptr; }
+
+http_url &http_url::operator=(http_url &&rhs)
+{
+ _url = rhs._url;
Review Comment:
Should free `_url` at first?
Then add a test to check there is no memory leak.
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...) \
+ do { \
+ const auto code = (expr); \
+ if (dsn_unlikely(code != expected)) { \
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__), to_error_msg(code))); \
+ return dsn::error_s::make(to_error_code(code), msg); \
+ } \
+ } while (0)
+
+http_url::http_url() : _url(nullptr) {}
+
+http_url::~http_url() { free_curlu_object(); }
+
+http_url::http_url(http_url &&rhs) : _url(rhs._url) { rhs._url = nullptr; }
+
+http_url &http_url::operator=(http_url &&rhs)
+{
+ _url = rhs._url;
+ rhs._url = nullptr;
+ return *this;
+}
+
+dsn::error_s http_url::init()
+{
+ if (_url != nullptr) {
+ free_curlu_object();
+ }
+
+ _url = curl_url();
+ if (_url == nullptr) {
+ return dsn::error_s::make(dsn::ERR_CURL_FAILED, "fail to initialize curl url");
+ }
Review Comment:
Looks like the macro in https://github.com/apache/incubator-pegasus/pull/1706/files#diff-4cc4ac5293103b26f79ab90be5de1bf7bbe24a37bbdcda43b2f85bbdd8be360eR39, how about submit the macro at first? @Samunroyu
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(http_client): add http_url class to help build URLs for http client [incubator-pegasus]
Posted by "Samunroyu (via GitHub)" <gi...@apache.org>.
Samunroyu commented on code in PR #1843:
URL: https://github.com/apache/incubator-pegasus/pull/1843#discussion_r1450198636
##########
src/http/http_client.cpp:
##########
@@ -34,6 +34,135 @@ DSN_DEFINE_uint32(http,
"The maximum time in milliseconds that you allow the libcurl transfer operation "
"to complete");
+#define RETURN_IF_CURL_NOT(expr, expected, ...) \
+ do { \
+ const auto code = (expr); \
+ if (dsn_unlikely(code != expected)) { \
+ std::string msg(fmt::format("{}: {}", fmt::format(__VA_ARGS__), to_error_msg(code))); \
+ return dsn::error_s::make(to_error_code(code), msg); \
+ } \
+ } while (0)
+
+http_url::http_url() : _url(nullptr) {}
+
+http_url::~http_url() { free_curlu_object(); }
+
+http_url::http_url(http_url &&rhs) : _url(rhs._url) { rhs._url = nullptr; }
+
+http_url &http_url::operator=(http_url &&rhs)
+{
+ _url = rhs._url;
+ rhs._url = nullptr;
+ return *this;
+}
+
+dsn::error_s http_url::init()
+{
+ if (_url != nullptr) {
+ free_curlu_object();
+ }
+
+ _url = curl_url();
+ if (_url == nullptr) {
+ return dsn::error_s::make(dsn::ERR_CURL_FAILED, "fail to initialize curl url");
+ }
Review Comment:
> Looks like the macro in https://github.com/apache/incubator-pegasus/pull/1706/files#diff-4cc4ac5293103b26f79ab90be5de1bf7bbe24a37bbdcda43b2f85bbdd8be360eR39, how about submit the macro at first? @Samunroyu
Ok.It's already merged. PR:: https://github.com/apache/incubator-pegasus/pull/1849/files
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(http_client): add http_url class to help build URLs for http client [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan merged PR #1843:
URL: https://github.com/apache/incubator-pegasus/pull/1843
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org