Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add migration_finalization_timeout_ms flag #4301

Merged
merged 1 commit into from
Dec 13, 2024
+9 −5
Merged

feat: add migration_finalization_timeout_ms flag #4301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/server/cluster/incoming_slot_migration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
#include "server/main_service.h"
#include "util/fibers/synchronization.h"

ABSL_DECLARE_FLAG(int, slot_migration_connection_timeout_ms);
ABSL_DECLARE_FLAG(int, migration_finalization_timeout_ms);

namespace dfly::cluster {

Expand Down Expand Up @@ -173,7 +173,7 @@ void IncomingSlotMigration::Pause(bool pause) {
bool IncomingSlotMigration::Join(long attempt) {
const absl::Time start = absl::Now();
const absl::Duration timeout =
absl::Milliseconds(absl::GetFlag(FLAGS_slot_migration_connection_timeout_ms));
absl::Milliseconds(absl::GetFlag(FLAGS_migration_finalization_timeout_ms));

while (true) {
const absl::Time now = absl::Now();
Expand Down Expand Up @@ -209,7 +209,7 @@ void IncomingSlotMigration::Stop() {
// we need to Join the migration process to prevent data corruption
const absl::Time start = absl::Now();
const absl::Duration timeout =
absl::Milliseconds(absl::GetFlag(FLAGS_slot_migration_connection_timeout_ms));
absl::Milliseconds(absl::GetFlag(FLAGS_migration_finalization_timeout_ms));

while (true) {
const absl::Time now = absl::Now();
Expand Down
7 changes: 5 additions & 2 deletions src/server/cluster/outgoing_slot_migration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@
#include "server/server_family.h"
#include "util/fibers/synchronization.h"

ABSL_FLAG(int, slot_migration_connection_timeout_ms, 5000, "Timeout for network operations");
ABSL_FLAG(int, slot_migration_connection_timeout_ms, 2000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you synced with @romange about the new flag? I am asking cause we usually discuss before we introduce a new flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have discussed it with @adiholden

"Connection creating timeout for migration operations");
ABSL_FLAG(int, migration_finalization_timeout_ms, 30000,
BorysTheDev marked this conversation as resolved.
Show resolved Hide resolved
"Timeout for migration finalization operation");
Comment on lines +24 to +26
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a little bit more in the description about what these flags do because Connection creating timeout for migration operations seems vague. I would expect with that description a timeout would trigger at 2000 ms which would include the 30000 ms for the migration to finalize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what else should be added. migration finalization is a several-step algorithm and it needs time

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can always reiterate on the descriptions so I leave this for you and the rest of team to decide


using namespace std;
using namespace facade;
Expand Down Expand Up @@ -333,7 +336,7 @@ bool OutgoingMigration::FinalizeMigration(long attempt) {

const absl::Time start = absl::Now();
const absl::Duration timeout =
absl::Milliseconds(absl::GetFlag(FLAGS_slot_migration_connection_timeout_ms));
absl::Milliseconds(absl::GetFlag(FLAGS_migration_finalization_timeout_ms));
while (true) {
const absl::Time now = absl::Now();
const absl::Duration passed = now - start;
Expand Down
1 change: 1 addition & 0 deletions src/server/main_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,7 @@ void Service::Init(util::AcceptServer* acceptor, std::vector<facade::Listener*>

config_registry.RegisterMutable("replica_partial_sync");
config_registry.RegisterMutable("replication_timeout");
config_registry.RegisterMutable("migration_finalization_timeout_ms");
config_registry.RegisterMutable("table_growth_margin");
config_registry.RegisterMutable("tcp_keepalive");
config_registry.RegisterMutable("managed_service_info");
Expand Down
Loading