-
Notifications
You must be signed in to change notification settings - Fork 899
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.
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(cluster): add tx execution in cluster_shard_migration #2385
Conversation
045654e
to
95a315c
Compare
@@ -19,6 +19,7 @@ | |||
namespace dfly { | |||
|
|||
using SlotId = uint16_t; | |||
// TODO consider to use bit set or some more compact way to store SlotId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do use a bitset below (look for my_slots_
). I guess we should use SlotSet
when it's common to have a small number of slots, whereas a bitset should be used for a large number of slots (as it always takes up 2kb per thread)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think about vector for everywhere the most optimal form to my mind
} | ||
} | ||
|
||
void ClusterShardMigration::ExecuteTxWithNoShardSync(TransactionData&& tx_data, Context* cntx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this code copied from somewhere?
I was hoping we could maybe reuse some of the replica code for executing such transactions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try at least to re-use it, that's 40 lines of confusing execution logic below 😶
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I also think about it, but I need a little more understanding of what we really need. I've already added tx-executor for such a purpose but it's a little harder for other code
52a5f6f
to
099bd32
Compare
refactor(replication): move code that is common for cluster and replica into separate file
2ee0df0
to
4f3bfb1
Compare
std::lock_guard lck(migration_mu_); | ||
auto migration_it = | ||
std::find_if(incoming_migrations_jobs_.begin(), incoming_migrations_jobs_.end(), | ||
[sync_id](const auto& el) { return el->GetSyncId() == sync_id; }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd avoid using templated lambdas, if possible.
Unlike regular auto
, these generic lambdas make it harder to navigate code sometimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a problem when it is used as an argument for a generic algorithm. auto just for a shorter name
return; | ||
} | ||
CHECK(tx_data.shard_cnt <= 1); // we don't support sync for multishard execution | ||
if (!tx_data.IsGlobalCmd()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think it's better to use positive logic first, then negative
In this case: if (IsGlobal()) { ... } else { ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to leave the code as is for two reasons. compilers in most cases optimize the first branch that is more important for us and for now we shouldn't have global cmds
slot_migrations_[shard_id]->Start(dest); | ||
} | ||
|
||
MigrationState OutgoingMigration::GetState() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call this GetMinState()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer GetState() because I want to show the state of the migration process but not of one flow even if migration process state is a min of all flows state)
@@ -0,0 +1,134 @@ | |||
// Copyright 2024, DragonflyDB authors. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there changes in the file which are not simple cut-paste?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of lines I've added one parameter to a InsertTxToSharedMap method
feat(cluster): add data migrations from a source node to a target node and set state into STABLE_SYNC after full-sync-cut command
implements #2447
refactor(replication): move code that is common for cluster and replica into a separate file