You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "SharonIV0x86 (via GitHub)" <gi...@apache.org> on 2025/04/21 14:59:48 UTC
[PR] chore(log): replace logging calls in ``cluster/*`` [kvrocks]
SharonIV0x86 opened a new pull request, #2894:
URL: https://github.com/apache/kvrocks/pull/2894
Related to #2889
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#discussion_r2059524883
##########
src/cluster/replication.cc:
##########
@@ -962,14 +954,14 @@ Status ReplicationThread::fetchFiles(int sock_fd, const std::string &dir, const
UniqueEvbuf evbuf;
for (unsigned i = 0; i < files.size(); i++) {
- DLOG(INFO) << "[fetch] Start to fetch file " << files[i];
+ info("[fetch] Start to fetch file {}", files[i]);
Review Comment:
```suggestion
debug("[fetch] Start to fetch file {}", files[i]);
```
##########
src/cluster/replication.cc:
##########
@@ -962,14 +954,14 @@ Status ReplicationThread::fetchFiles(int sock_fd, const std::string &dir, const
UniqueEvbuf evbuf;
for (unsigned i = 0; i < files.size(); i++) {
- DLOG(INFO) << "[fetch] Start to fetch file " << files[i];
+ info("[fetch] Start to fetch file {}", files[i]);
s = fetchFile(sock_fd, evbuf.get(), dir, files[i], crcs[i], fn, ssl);
if (!s.IsOK()) {
s = Status(Status::NotOK, "fetch file err: " + s.Msg());
- LOG(WARNING) << "[fetch] Fail to fetch file " << files[i] << ", err: " << s.Msg();
+ warn("[fetch] Fail to fetch file {}, err: {}", files[i], s.Msg());
break;
}
- DLOG(INFO) << "[fetch] Succeed fetching file " << files[i];
+ info("[fetch] Succeed fetching file {}", files[i]);
Review Comment:
```suggestion
debug("[fetch] Succeed fetching file {}", files[i]);
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#issuecomment-2829447325
You can try `fmt::streamed(SSLErrors{})`
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#issuecomment-2830798040
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2894) **Quality Gate failed**
Failed conditions
 [38.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2894&metric=new_coverage&view=list) (required ≥ 50%)
 [D Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2894) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2894)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#discussion_r2053230384
##########
src/cluster/cluster.cc:
##########
@@ -130,7 +130,7 @@ Status Cluster::SetSlotRanges(const std::vector<SlotRange> &slot_ranges, const s
if (migrated_slots_.count(slot) > 0) {
auto s = srv_->slot_migrator->ClearKeysOfSlotRange(ctx, kDefaultNamespace, SlotRange::GetPoint(slot));
if (!s.ok()) {
- LOG(ERROR) << "failed to clear data of migrated slot: " << s.ToString();
+ error("failed to clear data of migrated slot {}", s.ToString());
Review Comment:
```suggestion
error("failed to clear data of migrated slot: {}", s.ToString());
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#discussion_r2059524416
##########
src/cluster/replication.cc:
##########
@@ -203,7 +203,7 @@ void ReplicationThread::CallbacksStateMachine::SetWriteCB(bufferevent *bev, buff
void ReplicationThread::CallbacksStateMachine::ReadWriteCB(bufferevent *bev) {
LOOP_LABEL:
assert(handler_idx_ <= handlers_.size());
- DLOG(INFO) << "[replication] Execute handler[" << getHandlerName(handler_idx_) << "]";
+ info("[replication] Execute handler[{}]", getHandlerName(handler_idx_));
Review Comment:
```suggestion
debug("[replication] Execute handler[{}]", getHandlerName(handler_idx_));
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#discussion_r2053232216
##########
src/cluster/replication.cc:
##########
@@ -84,12 +84,12 @@ Status FeedSlaveThread::Start() {
void FeedSlaveThread::Stop() {
stop_ = true;
- LOG(WARNING) << "Slave thread was terminated, would stop feeding the slave: " << conn_->GetAddr();
+ warn("Slave thread was terminated, would stop feeding the slave: {}", conn_->GetAddr);
Review Comment:
```suggestion
warn("Slave thread was terminated, would stop feeding the slave: {}", conn_->GetAddr());
```
Hmm...
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#issuecomment-2829358976
Some syntax errors. Did you try to compile the code after these changes?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#discussion_r2053231034
##########
src/cluster/cluster.cc:
##########
@@ -261,16 +261,15 @@ Status Cluster::SetMasterSlaveRepl() {
std::shared_ptr<ClusterNode> master = it->second;
auto s = srv_->AddMaster(master->host, master->port, false);
if (!s.IsOK()) {
- LOG(WARNING) << "SLAVE OF " << master->host << ":" << master->port
- << " wasn't enabled by cluster topology setting, encounter error: " << s.Msg();
+ warn("SLAVE OF {} : {} wasn't enabled by cluster topology setting, encounter error: {}", master->host, master->port, s.Msg());
Review Comment:
```suggestion
warn("SLAVE OF {}:{} wasn't enabled by cluster topology setting, encounter error: {}", master->host, master->port, s.Msg());
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "SharonIV0x86 (via GitHub)" <gi...@apache.org>.
SharonIV0x86 commented on PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#issuecomment-2829411606
@PragmaTwice Should work now.
Apologies for the earlier mistakes, I was in a bit of a rush while working on the PR, which I realize I shouldn't have been. I'll make sure to be more careful going forward.
On a another note, I had a quick question regarding my GSoC proposal on database backups. Since the [Kvrocks community page](https://kvrocks.apache.org/community/#zulip) mentions that all important decisions should go through the mailing list, do I need to share my proposal there as well for visibility and feedback? Just want to make sure I'm following the right process. Thanks.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#discussion_r2059526851
##########
src/cluster/replication.cc:
##########
@@ -737,27 +733,26 @@ ReplicationThread::CBState ReplicationThread::fullSyncReadCB(bufferevent *bev) {
s = storage_->RestoreFromCheckpoint();
}
if (!s.IsOK()) {
- LOG(ERROR) << "[replication] Failed to restore backup while " + s.Msg() + ", restart fullsync";
+ error("[replication] Failed to restore backup while {}, restart fullsync", s.Msg());
post_fullsync_cb_();
return CBState::RESTART;
}
- LOG(INFO) << "[replication] Succeeded restoring the backup, fullsync was finish";
+ info("[replication] Succeeded restoring the backup, fullsync was finish");
post_fullsync_cb_();
// It needs to reload namespaces from DB after the full sync is done,
// or namespaces are not visible in the replica.
s = srv_->GetNamespace()->LoadAndRewrite();
if (!s.IsOK()) {
- LOG(ERROR) << "[replication] Failed to load and rewrite namespace: " << s.Msg();
+ error("[replication] Failed to load and rewrite namespace: {}", s.Msg());
}
// Switch to psync state machine again
psync_steps_.Start();
return CBState::QUIT;
}
}
-
- LOG(ERROR) << "Should not arrive here";
+ error("Should not arrive here");
assert(false);
return CBState::QUIT;
Review Comment:
```suggestion
unreachable();
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "SharonIV0x86 (via GitHub)" <gi...@apache.org>.
SharonIV0x86 commented on PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#issuecomment-2829360062
> Some syntax errors. Did you try to compile the code after these changes?
I did, ill look into it right now.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#discussion_r2053231890
##########
src/cluster/replication.cc:
##########
@@ -67,7 +67,7 @@ Status FeedSlaveThread::Start() {
pthread_sigmask(SIG_BLOCK, &mask, &omask);
auto s = util::SockSend(conn_->GetFD(), redis::RESP_OK, conn_->GetBufferEvent());
if (!s.IsOK()) {
- LOG(ERROR) << "failed to send OK response to the replica: " << s.Msg();
+ error("failed to send OK response to the replica {}", s.Msg());
Review Comment:
```suggestion
error("failed to send OK response to the replica: {}", s.Msg());
```
Why?
##########
src/cluster/replication.cc:
##########
@@ -67,7 +67,7 @@ Status FeedSlaveThread::Start() {
pthread_sigmask(SIG_BLOCK, &mask, &omask);
auto s = util::SockSend(conn_->GetFD(), redis::RESP_OK, conn_->GetBufferEvent());
if (!s.IsOK()) {
- LOG(ERROR) << "failed to send OK response to the replica: " << s.Msg();
+ error("failed to send OK response to the replica {}", s.Msg());
Review Comment:
```suggestion
error("failed to send OK response to the replica: {}", s.Msg());
```
Why to change the message?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#discussion_r2053230690
##########
src/cluster/cluster.cc:
##########
@@ -246,11 +246,11 @@ Status Cluster::SetMasterSlaveRepl() {
if (!s.IsOK()) {
return s.Prefixed("failed to remove master");
}
- LOG(INFO) << "MASTER MODE enabled by cluster topology setting";
+ info("MASTER MODE enabled by cluster topology setting");
if (srv_->slot_migrator && is_cluster_enabled && is_slave) {
// Slave -> Master
srv_->slot_migrator->SetStopMigrationFlag(false);
- LOG(INFO) << "Change server role to master, restart migration task";
+ info("Change server role to master, restart migration task")
Review Comment:
```suggestion
info("Change server role to master, restart migration task");
```
?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#discussion_r2059518141
##########
src/cluster/cluster.cc:
##########
@@ -130,7 +130,7 @@ Status Cluster::SetSlotRanges(const std::vector<SlotRange> &slot_ranges, const s
if (migrated_slots_.count(slot) > 0) {
auto s = srv_->slot_migrator->ClearKeysOfSlotRange(ctx, kDefaultNamespace, SlotRange::GetPoint(slot));
if (!s.ok()) {
- LOG(ERROR) << "failed to clear data of migrated slot: " << s.ToString();
+ error("failed to clear data of migrated slot {} ", s.ToString());
Review Comment:
```suggestion
error("failed to clear data of migrated slot: {}", s.ToString());
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "SharonIV0x86 (via GitHub)" <gi...@apache.org>.
SharonIV0x86 commented on PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#issuecomment-2829425216
```sh
implicit instantiation of undefined template 'fmt::detail::type_is_unformattable_for<SSLErrors, char>'
type_is_unformattable_for<T, char_type> _;
```
Seems to be an issue from here.
```cpp
error("Failed to construct SSL structure for new connection: {}", SSLErrors{});
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice merged PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] chore(log): replace logging calls in `cluster/*` [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2894:
URL: https://github.com/apache/kvrocks/pull/2894#discussion_r2053231362
##########
src/cluster/cluster.cc:
##########
@@ -374,7 +373,7 @@ Status Cluster::ImportSlotRange(redis::Connection *conn, const SlotRange &slot_r
conn->close_cb = [object_ptr = srv_->slot_import.get(), slot_range]([[maybe_unused]] int fd) {
auto s = object_ptr->StopForLinkError();
if (!s.IsOK()) {
- LOG(ERROR) << fmt::format("[import] Failed to stop importing slot(s) {}: {}", slot_range.String(), s.Msg());
+ error("[import] Failed to stop importing slot(s) {}:{}", slot_range.String(), s.Msg());
Review Comment:
```suggestion
error("[import] Failed to stop importing slot(s) {}: {}", slot_range.String(), s.Msg());
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org