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

Have client's TestOnly activate AsyncCompleter instead of BatchCompleter #441

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
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
32 changes: 26 additions & 6 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,17 @@ type Config struct {
// effect of making them slower. It should not be used outside of test
// suites.
//
// For example, queue maintenance services normally stagger their startup
// with a random jittered sleep so they don't all try to work at the same
// time. This is nice in production, but makes starting and stopping the
// client in a test case slower.
// Specific differences in the client's operation:
//
// * Queue maintenance services normally stagger their startup with a random
// jittered sleep so they don't all try to work at the same time. This is
// nice in production, but makes starting and stopping the client in a test
// case slower. TestOnly disables this jitter.
//
// * Jobs are normally completed in batches, with the completer pausing
// briefly on an incoming job in case more completions come in that can be
// added to the batch. The pause adds overhead to test cases waiting for
// jobs to complete, so TestOnly causes completions to run immediately.
TestOnly bool

// Workers is a bundle of registered job workers.
Expand Down Expand Up @@ -507,9 +514,22 @@ func NewClient[TTx any](driver riverdriver.Driver[TTx], config *Config) (*Client
return nil, errMissingDatabasePoolWithQueues
}

client.completer = jobcompleter.NewBatchCompleter(archetype, driver.GetExecutor(), nil)
// The batch completer is much faster when completing many jobs, but it
// pauses before completing jobs, waiting for more potential completions
// to come in which can be added to the batch. This is good for live
// environments but not as good for tests, where it adds some delay to
// every test case waiting for a job completion. With TestOnly on, use
// the async completer instead, which despite its name, starts
// incoming completions immediately by doing them in a goroutine.
if config.TestOnly {
client.completer = jobcompleter.NewAsyncCompleter(archetype, driver.GetExecutor(), nil)
} else {
client.completer = jobcompleter.NewBatchCompleter(archetype, driver.GetExecutor(), nil)
}
client.services = append(client.services, client.completer)

client.subscriptionManager = newSubscriptionManager(archetype, nil)
client.services = append(client.services, client.completer, client.subscriptionManager)
client.services = append(client.services, client.subscriptionManager)
Comment on lines +529 to +532

Choose a reason for hiding this comment

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

It would be cleaner to delete the first append and merge it into the second with append(client.services, client.completer, client.subscriptionManager). This potentially also avoids an unnecessary allocation as append only needs to allocate more memory once instead of potentially twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Realistically, if Go does need to grow the slice, it'll 2x the underlying size, so it's not going to make any difference. I'd take the small readability advantage instead of trying to micro-optimize here.


if driver.SupportsListener() {
// In poll only mode, we don't try to initialize a notifier that
Expand Down
Loading