Skip to content

Conversation

@kostasrim
Copy link
Contributor

@kostasrim kostasrim commented May 26, 2025

Helps with #5180

  • comment in partial sync
  • comment in partial sync specific test cases

Signed-off-by: kostas <kostas@dragonflydb.io>
@kostasrim kostasrim self-assigned this May 26, 2025

string eof_token;
std::string_view sync_type{"FULL"};
bool partial_sync = false;
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 can check again the sync_type also but I like the bool

error_code Journal::Close() {
VLOG(1) << "Journal::Close";

if (!journal_slice.IsOpen()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

redundant after we call journal_slice.Init once. Won't make any difference either if it's not open

@kostasrim kostasrim changed the title [wip] chore: bring back partial sync chore: bring back partial sync May 27, 2025
@kostasrim kostasrim marked this pull request as ready for review May 27, 2025 12:06
@kostasrim kostasrim requested a review from adiholden May 27, 2025 12:06
@kostasrim
Copy link
Contributor Author

@adiholden I just commented in the code old code + tests

I will follow up with another PR with tests


if (IsLSNDiffBellowThreshold(data.value().last_journal_LSNs, lsn_vec)) {
sync_type = "PARTIAL";
partial_sync = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

please rebase main branch this code is not up to date

@kostasrim kostasrim requested a review from adiholden June 4, 2025 10:56

sf_->journal()->StartInThread();

if (!partial_sync && seqid.has_value()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this condition should be else if
becuase we iether get seqid or last_master_id and last_master_lsn
So you can change to else if and remove the partial_sync bool

}
}

sf_->journal()->StartInThread();
Copy link
Contributor

Choose a reason for hiding this comment

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

also it makes sense to call StartInThread either before/after the 2 condition checks for partial sync

@kostasrim kostasrim requested a review from adiholden June 5, 2025 08:00
kostasrim added 2 commits June 5, 2025 13:56
Signed-off-by: kostas <kostas@dragonflydb.io>
@kostasrim kostasrim enabled auto-merge (squash) June 5, 2025 11:19
@kostasrim kostasrim merged commit 6a32dfd into main Jun 5, 2025
10 checks passed
@kostasrim kostasrim deleted the kpr2 branch June 5, 2025 12:07
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.

3 participants