Skip to content

Add product-drop-timeout parameter - #165

Draft
mpejcoch wants to merge 3 commits into
mainfrom
issue163_purge_old_products
Draft

Add product-drop-timeout parameter#165
mpejcoch wants to merge 3 commits into
mainfrom
issue163_purge_old_products

Conversation

@mpejcoch

Copy link
Copy Markdown
Contributor

No description provided.

@mpejcoch
mpejcoch requested review from havardf, johtoblan and vkbo August 12, 2021 13:44
Comment thread internal/server/api.go

// NewService creates a service struct, containing all that is needed for a mmsd server to run.
func NewService(templates *template.Template, eventsDB *sql.DB, stateDB *sql.DB, natsURL string) *Service {
func NewService(templates *template.Template, eventsDB *sql.DB, stateDB *sql.DB, natsURL string, productDropTimeout int) *Service {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think its time to consider a config struct or something for NewService, as the parameter list to the function is getting pretty long.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, I felt I was trying to push it a bit too far yesterday, now I have it confirmed :)

Comment thread cmd/mmsd/main.go
@@ -354,6 +359,8 @@ func startEventLoop(webService *server.Service) {
if err := webService.DeleteOldEvents(time.Now().AddDate(0, 0, -3)); err != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe purging events and unused products can be merged into one? So the api from the user of the service do not have to call both?

Like webService.CleanUpEvents, which again deletes old events and removes unused products?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea was to have separate configuration of how long to keep events and how long to keep products in the monitoring. It might not be necessary when I am thinking about it. Probably it is easier for the user to deal with only 1 parameter.

Purpose of this PR is to address an issue when products keep hanging in the metrics part forever.

We have discussed whether the product status and monitoring should even be part of MMS, there were practical reasons for this as it was easier to maintain on a multi-user machine. We should re-visit this when going over to k8s. It might make sense to split it up and have the functionality as a different component or even a system. The same plan we have with NATS.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Keeping two separate config values for product and old events might make sense, but my comment was more about having just one method call for doing cleanup and this method would handle both product and event cleanup. But if you think its better to keep both config values and cleanup method call separate I won't push it :)

@mpejcoch
mpejcoch marked this pull request as draft August 13, 2021 08:08
@mortenwh mortenwh added this to the Full project milestone Aug 22, 2021
@mpejcoch mpejcoch modified the milestones: Full project, Q3 2021 sprint Aug 23, 2021
@mortenwh mortenwh added the MMS label Aug 24, 2021
@mortenwh mortenwh modified the milestones: Q3 2021 sprint, Full project Sep 14, 2021
@mortenwh mortenwh removed this from the Full project milestone Feb 16, 2024
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.

3 participants