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.

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

Dual channel replication should not update lastbgsave_status when transfer error #811

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

enjoy-binbin
Copy link
Member

Currently lastbgsave_status is used in bgsave or disk-replication,
and the target is the disk. In #60, we update it when transfer error,
i think it is mainly used in tests, so we can use log to replace it.

It changes lastbgsave_status to err in this case, but it is strange
that it does not set ok or err in the above if and the following else.
Also noted this will affect stop-writes-on-bgsave-error.

…nsfer error

Currently lastbgsave_status is used in bgsave or disk-replication,
and the target is the disk. In valkey-io#60, we update it when transfer error,
i think it is mainly used in tests, so we can use log to replace it.

It changes lastbgsave_status to err in this case, but it is strange
that it does not set ok or err in the above if and the following else.
Also noted this will affect stop-writes-on-bgsave-error.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin
Copy link
Member Author

valkey-io/valkey-doc#158

I mentioned here that we have some mixed usage of these fields for disk-based or diskless.
I think we may need to set up some of the same fields separately for diskless.

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.32%. Comparing base (14e09e9) to head (b8d3809).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #811      +/-   ##
============================================
- Coverage     70.36%   70.32%   -0.05%     
============================================
  Files           112      112              
  Lines         61275    61274       -1     
============================================
- Hits          43115    43088      -27     
- Misses        18160    18186      +26     
Files Coverage Δ
src/rdb.c 75.79% <100.00%> (-0.30%) ⬇️

... and 10 files with indirect coverage changes

@madolson madolson requested review from ranshid and naglera July 23, 2024 21:26
src/rdb.c Show resolved Hide resolved
@madolson madolson requested a review from naglera August 7, 2024 17:42
Copy link
Contributor

@naglera naglera left a comment

Choose a reason for hiding this comment

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

Approved, adding this AI for a pemament solution
#878

@enjoy-binbin
Copy link
Member Author

@madolson i see #837 contain this PR (commit), lets merge this PR first?

@madolson madolson merged commit 929283f into valkey-io:unstable Aug 12, 2024
20 checks passed
@madolson madolson added the release-notes This issue should get a line item in the release notes label Aug 12, 2024
@enjoy-binbin enjoy-binbin deleted the fix_lastbgsave_status branch August 13, 2024 00:00
mapleFU pushed a commit to mapleFU/valkey that referenced this pull request Aug 21, 2024
…nsfer error (valkey-io#811)

Currently lastbgsave_status is used in bgsave or disk-replication,
and the target is the disk. In valkey-io#60, we update it when transfer error,
i think it is mainly used in tests, so we can use log to replace it.

It changes lastbgsave_status to err in this case, but it is strange
that it does not set ok or err in the above if and the following else.
Also noted this will affect stop-writes-on-bgsave-error.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: mwish <maplewish117@gmail.com>
mapleFU pushed a commit to mapleFU/valkey that referenced this pull request Aug 22, 2024
…nsfer error (valkey-io#811)

Currently lastbgsave_status is used in bgsave or disk-replication,
and the target is the disk. In valkey-io#60, we update it when transfer error,
i think it is mainly used in tests, so we can use log to replace it.

It changes lastbgsave_status to err in this case, but it is strange
that it does not set ok or err in the above if and the following else.
Also noted this will affect stop-writes-on-bgsave-error.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: mwish <maplewish117@gmail.com>
madolson pushed a commit that referenced this pull request Sep 2, 2024
…nsfer error (#811)

Currently lastbgsave_status is used in bgsave or disk-replication,
and the target is the disk. In #60, we update it when transfer error,
i think it is mainly used in tests, so we can use log to replace it.

It changes lastbgsave_status to err in this case, but it is strange
that it does not set ok or err in the above if and the following else.
Also noted this will affect stop-writes-on-bgsave-error.

Signed-off-by: Binbin <binloveplay1314@qq.com>
madolson pushed a commit that referenced this pull request Sep 3, 2024
…nsfer error (#811)

Currently lastbgsave_status is used in bgsave or disk-replication,
and the target is the disk. In #60, we update it when transfer error,
i think it is mainly used in tests, so we can use log to replace it.

It changes lastbgsave_status to err in this case, but it is strange
that it does not set ok or err in the above if and the following else.
Also noted this will affect stop-writes-on-bgsave-error.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes This issue should get a line item in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants