chore: cleanup notifier

[achettyiitr]

Description

• Introducing repository and model for Notifier.
• Renamed it to notifier instead of pgNotifier since we don't need to expose the implementation details like it is implemented for Postgres.
• Removing globals.
• Removed notifier.Init().
• Introduced background errgroup to gracefully shut down the goroutines being created during track batch and subscribe.

Linear Ticket

• https://linear.app/rudderstack/issue/PIPE-169/pgnotifier-init-cleanup
• https://linear.app/rudderstack/issue/PIPE-245/notifier-introduce-repository-and-model

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]

Codecov Report

Patch coverage: 82.13% and project coverage change: -0.01% ⚠️

Comparison is base (b06dc36) 68.98% compared to head (2d43e31) 68.97%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3867      +/-   ##
==========================================
- Coverage   68.98%   68.97%   -0.01%     
==========================================
  Files         351      352       +1     
  Lines       52816    52971     +155     
==========================================
+ Hits        36435    36538     +103     
- Misses      14067    14117      +50     
- Partials     2314     2316       +2     

Files Changed	Coverage Δ
runner/runner.go	68.37% <ø> (-0.14%)	⬇️
warehouse/http.go	77.58% <0.00%> (ø)
warehouse/router.go	88.45% <33.33%> (+1.23%)	⬆️
warehouse/jobs/runner.go	59.56% <57.14%> (-1.90%)	⬇️
warehouse/internal/loadfiles/loadfiles.go	72.95% <68.88%> (-2.49%)	⬇️
warehouse/slave_worker.go	79.88% <77.41%> (-0.68%)	⬇️
services/notifier/notifier.go	80.24% <80.24%> (ø)
warehouse/warehouse.go	78.32% <83.33%> (+0.27%)	⬆️
services/notifier/repo.go	89.47% <89.47%> (ø)
warehouse/jobs/http.go	76.63% <100.00%> (ø)
... and 3 more

... and 13 files with indirect coverage changes

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

[lvrach]

why not:

Suggested change

	n.config.host = n.conf.GetString("PGNOTIFIER_DB_HOST", "localhost")
	n.config.user = n.conf.GetString("PGNOTIFIER_DB_USER", "ubuntu")
	n.config.database = n.conf.GetString("PGNOTIFIER_DB_NAME", "ubuntu")
	n.config.port = n.conf.GetInt("PGNOTIFIER_DB_PORT", 5432)
	n.config.password = n.conf.GetString("PGNOTIFIER_DB_PASSWORD", "ubuntu")
	n.config.sslMode = n.conf.GetString("PGNOTIFIER_DB_SSL_MODE", "disable")
	n.config.maxAttempt = n.conf.GetInt("PgNotifier.maxAttempt", 3)
	n.config.maxOpenConnections = n.conf.GetInt("PgNotifier.maxOpenConnections", 20)
	n.config.shouldForceSetLowerVersion = n.conf.GetBool("SQLMigrator.forceSetLowerVersion", true)
	n.config.trackBatchInterval = n.conf.GetDuration("PgNotifier.trackBatchIntervalInS", 2, time.Second)
	n.config.queryTimeout = n.conf.GetDuration("Warehouse.pgNotifierQueryTimeout", 5, time.Minute)
	n.config = config{
	host: n.conf.GetString("PGNOTIFIER_DB_HOST", "localhost")
	user: n.conf.GetString("PGNOTIFIER_DB_USER", "ubuntu")
	database: n.conf.GetString("PGNOTIFIER_DB_NAME", "ubuntu")
	....

[achettyiitr]

This will require creating a new standalone struct which I am not sure is required here. We are okay with an anonymous structure here.

[lvrach]

I would suggest the following structure:

moving this file to notifier/notifier.go containing models part of a the common interface, shared between multiple implementations. Having to access them from model package seems a bit counter intuitive for a library.

move notifier/notifier.go -> notifier/pgnotifer.go and instead of notifier.New() -> notifier.NewPG() -- since it a specific implementation.

[achettyiitr]

moving this file to notifier/notifier.go containing models part of a the common interface, shared between multiple implementations. Having to access them from model package seems a bit counter intuitive for a library.

Makes sense. Since it's a package, it should provide a single interface for accessing it. I was initially targeting a repo package and since this model will be shared so moved models to a separate package. Moving everything to the notifier package.

[achettyiitr]

move notifier/notifier.go -> notifier/pgnotifer.go and instead of notifier.New() -> notifier.NewPG() -- since it a specific implementation

I am not sure whether the consumers need to be aware of the implementation details like whether it is implemented for Postgres, Pulsar, or something else.

[fracasula]

I left a bunch of questions 🙂

[fracasula]

Good stuff 😎 👍