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

chore: option to pass *config.Config to jobsdb #3764

Merged
merged 9 commits into from
Aug 22, 2023
Merged

Conversation

Sidddddarth
Copy link
Member

@Sidddddarth Sidddddarth commented Aug 19, 2023

Description

Option(Optfunc) to have jobsdb-handle-specific config, which falls back to config.Default if not passed.
This can help avoid configuration mixups between tests, each jobsdb handle can have specific config to itself, and also to test actual runtime behaviour by changing config values instead of setting jobsdb.Handle's internal fields.

Also moved and grouped jobsdb config fields around to help with readability. eg.: it wasn't entirely clear(to me at first glance) how the number of maxOpenConnections were being set in case of a write/read/readWrite jobsdb.

Additionally:
misc.GetConnectionString now takes in *config.Config as a parameter, using it to fetch db connection details.

Linear Ticket

jobsdb handle specific config

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Patch coverage: 84.97% and project coverage change: +0.01% 🎉

Comparison is base (901c69e) 68.69% compared to head (e5c564a) 68.70%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3764      +/-   ##
==========================================
+ Coverage   68.69%   68.70%   +0.01%     
==========================================
  Files         345      345              
  Lines       51652    51649       -3     
==========================================
+ Hits        35480    35486       +6     
+ Misses      13884    13877       -7     
+ Partials     2288     2286       -2     
Files Changed Coverage Δ
app/apphandlers/processorAppHandler.go 8.56% <0.00%> (ø)
processor/stash/stash.go 52.51% <0.00%> (ø)
runner/runner.go 69.16% <ø> (-0.13%) ⬇️
services/validators/envValidator.go 50.45% <50.00%> (ø)
warehouse/warehouse.go 55.14% <50.00%> (ø)
utils/misc/dbutils.go 33.33% <53.84%> (ø)
jobsdb/migration.go 69.85% <77.77%> (-0.64%) ⬇️
jobsdb/backup.go 69.25% <80.76%> (ø)
jobsdb/jobsdb.go 72.67% <91.85%> (+0.06%) ⬆️
app/apphandlers/embeddedAppHandler.go 76.67% <100.00%> (ø)
... and 6 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sidddddarth Sidddddarth marked this pull request as ready for review August 21, 2023 07:02
jobsdb/jobsdb.go Outdated Show resolved Hide resolved
jobsdb/integration_test.go Outdated Show resolved Hide resolved
jobsdb/migration_test.go Outdated Show resolved Hide resolved
archiver/archiver_test.go Outdated Show resolved Hide resolved
jobsdb/jobsdb.go Outdated Show resolved Hide resolved
@Sidddddarth Sidddddarth force-pushed the chore.jobsdbConfigDep branch 2 times, most recently from d84ac94 to cf30098 Compare August 21, 2023 14:55
utils/misc/dbutils.go Outdated Show resolved Hide resolved
archiver/archiver_test.go Outdated Show resolved Hide resolved
archiver/archiver_test.go Outdated Show resolved Hide resolved
Comment on lines -160 to -162
TriggerAddNewDS: func() <-chan time.Time {
return triggerAddNewDS
},
Copy link
Contributor

Choose a reason for hiding this comment

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

how are we overriding jobsdb now so that it doesn't trigger the addNewDS loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

It wasn't being used anywhere in the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be that the test wanted to simply disable this trigger?

Copy link
Member Author

Choose a reason for hiding this comment

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

true
didn't think of it that way😅
Fortunately this didn't affect this particular test.
But in the future with the config dependency, we could set the addDSLoop-timer to a sufficiently large duration.

processor/manager_test.go Outdated Show resolved Hide resolved
warehouse/router_test.go Outdated Show resolved Hide resolved
jobsdb/jobsdb.go Outdated Show resolved Hide resolved
jobsdb/jobsdb.go Outdated Show resolved Hide resolved
@atzoum
Copy link
Contributor

atzoum commented Aug 22, 2023

thank you @Sidddddarth! with respect to the description of this pull request, I believe it deserves some more attention to include all introduced changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants