Skip to content

Conversation

@SharonIV0x86
Copy link
Contributor

Related to #2889

@SharonIV0x86 SharonIV0x86 changed the title chore(log): replace logging calls in cluster/* chore(log): replace logging calls in cluster/* Apr 22, 2025
@SharonIV0x86 SharonIV0x86 marked this pull request as ready for review April 22, 2025 02:23
@PragmaTwice PragmaTwice changed the title chore(log): replace logging calls in cluster/* chore(log): replace logging calls in cluster/* Apr 22, 2025
// 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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
info("Change server role to master, restart migration task")
info("Change server role to master, restart migration task");

?

Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review every change you made. So many mistakes..

@SharonIV0x86 SharonIV0x86 marked this pull request as draft April 22, 2025 03:39
made small adjustments

replaced DLOG calls

adjusted clang formatting

chore(ci): mark the branch 2.12 as a protected branch (apache#2895)

missing placeholder
@SharonIV0x86 SharonIV0x86 force-pushed the chore/replace-logging-cluster branch from 09cbf7b to aa9c553 Compare April 23, 2025 16:56
@SharonIV0x86 SharonIV0x86 marked this pull request as ready for review April 23, 2025 17:03
PragmaTwice
PragmaTwice previously approved these changes Apr 25, 2025
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now : )

@PragmaTwice
Copy link
Member

Some syntax errors. Did you try to compile the code after these changes?

@SharonIV0x86
Copy link
Contributor Author

Some syntax errors. Did you try to compile the code after these changes?

I did, ill look into it right now.

@SharonIV0x86
Copy link
Contributor Author

@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 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.

@SharonIV0x86
Copy link
Contributor Author

SharonIV0x86 commented Apr 25, 2025

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.

error("Failed to construct SSL structure for new connection: {}", SSLErrors{});

Doing this might work? as it says here gabime/spdlog#2041 (comment)

std::ostringstream oss;
oss << SSLErrors{};
error("Failed to construct SSL structure for new connection: {}", oss.str());

@PragmaTwice
Copy link
Member

You can try fmt::streamed(SSLErrors{})

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
38.1% Coverage on New Code (required ≥ 50%)
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@PragmaTwice PragmaTwice merged commit 858d7f2 into apache:unstable Apr 26, 2025
34 of 35 checks passed
@SharonIV0x86 SharonIV0x86 deleted the chore/replace-logging-cluster branch May 1, 2025 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants