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

CBG-4370 create a cancel context inside BlipSyncContext #7201

Merged
merged 11 commits into from
Jan 17, 2025
Merged

Conversation

torcolvin
Copy link
Collaborator

@torcolvin torcolvin commented Nov 18, 2024

CBG-4370 create a cancel context inside BlipSyncContext

This cancel context allows a forceable closure of the underlying blip connection.

In the case there is a continuous pull replication and there is an error on the changes feed, the only way to stop the pull replication is to shut down the connection. CBL clients do not listen to unsolicited error messages.

This leverages 6317f19

I added some leaky bucket code to be able to trigger the error.

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

This cancel context allows a forceable closure of the underlying blip
connection.

In the case there is a continuous pull replication and there is an error
on the changes feed, the only way to stop the pull replication is to
shut down the connection. CBL clients do not listen to unsolicited error
messages.
Copy link
Collaborator

@adamcfraser adamcfraser left a comment

Choose a reason for hiding this comment

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

A few comments/questions.

db/changes.go Show resolved Hide resolved
db/blip_handler.go Show resolved Hide resolved
db/active_replicator.go Show resolved Hide resolved
@@ -34,7 +34,10 @@ const (

var ErrClosedBLIPSender = errors.New("use of closed BLIP sender")

func NewBlipSyncContext(ctx context.Context, bc *blip.Context, db *Database, contextID string, replicationStats *BlipSyncStats) *BlipSyncContext {
func NewBlipSyncContext(ctx context.Context, bc *blip.Context, db *Database, contextID string, replicationStats *BlipSyncStats, ctxCancelFunc context.CancelFunc) (*BlipSyncContext, error) {
if ctxCancelFunc == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this required for cases where the BlipSyncContext is for an active peer (ISGR)? Closing the connection in that case feels like a different problem than the passive peer case.


base.SetUpTestLogging(t, base.LevelInfo, base.KeyHTTP, base.KeySync, base.KeySyncMsg, base.KeyChanges, base.KeyCache)
btcRunner := NewBlipTesterClientRunner(t)
var shouldChanenlQueryError atomic.Bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var shouldChanenlQueryError atomic.Bool
var shouldChannelQueryError atomic.Bool

rest/blip_sync.go Show resolved Hide resolved
@torcolvin torcolvin requested a review from adamcfraser December 2, 2024 14:17
@torcolvin torcolvin assigned adamcfraser and unassigned torcolvin Dec 2, 2024
Copy link
Collaborator

@adamcfraser adamcfraser left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for the additional ISGR push test, that was informative.

@adamcfraser adamcfraser merged commit 0efa2a4 into main Jan 17, 2025
40 checks passed
@adamcfraser adamcfraser deleted the CBG-4370 branch January 17, 2025 21:32
torcolvin added a commit that referenced this pull request Jan 21, 2025
* CBG-4370 create a cancel context inside BlipSyncContext

This cancel context allows a forceable closure of the underlying blip
connection.

In the case there is a continuous pull replication and there is an error
on the changes feed, the only way to stop the pull replication is to
shut down the connection. CBL clients do not listen to unsolicited error
messages.

* Avoid refactoring with the change

* pass lint

* test active replicator reconnection

* test fixup

* lint correctly

* PR comments

* Acquire read lock to present data race

* switch query to use all docs index
torcolvin added a commit that referenced this pull request Jan 21, 2025
* CBG-4370 create a cancel context inside BlipSyncContext

This cancel context allows a forceable closure of the underlying blip
connection.

In the case there is a continuous pull replication and there is an error
on the changes feed, the only way to stop the pull replication is to
shut down the connection. CBL clients do not listen to unsolicited error
messages.

* Avoid refactoring with the change

* pass lint

* test active replicator reconnection

* test fixup

* lint correctly

* PR comments

* Acquire read lock to present data race

* switch query to use all docs index
bbrks pushed a commit that referenced this pull request Jan 22, 2025
…ext (#7314)

* CBG-4370 preqreq deduplicate active replicator pull changes (#7211)

* CBG-4370 create a cancel context inside BlipSyncContext (#7201)

* CBG-4370 create a cancel context inside BlipSyncContext

This cancel context allows a forceable closure of the underlying blip
connection.

In the case there is a continuous pull replication and there is an error
on the changes feed, the only way to stop the pull replication is to
shut down the connection. CBL clients do not listen to unsolicited error
messages.

* Avoid refactoring with the change

* pass lint

* test active replicator reconnection

* test fixup

* lint correctly

* PR comments

* Acquire read lock to present data race

* switch query to use all docs index

* fix lint issue
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