Skip to content

Conversation

@lelenanam
Copy link
Contributor

@lelenanam lelenanam commented Feb 3, 2018

Add deleted_at to GetConfig, findConfigs and findRulesConfigs functions so they can return actual value of deleted_at for later use in ruler. Otherwise, ruler never discoveries deleted configs and never delete them from the map.

By default deleted_at column in DB set to NULL so we scan this into pq.NullTime variable (because we cannot store nil value into type time.Time).
To restore config we set deleted_at column in DB to NULL.

fixes https://github.com/weaveworks/service-conf/issues/1859

@lelenanam lelenanam requested review from awh and bboreham February 3, 2018 04:15
@bboreham
Copy link
Contributor

bboreham commented Feb 6, 2018

Please give more explanation of the issue - this is a public repo so you can't just link to a private one.
And there isn't any information in that issue anyway about what really went wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this. Why can't the zero time stand for "null" ?

Copy link
Contributor Author

@lelenanam lelenanam Feb 6, 2018

Choose a reason for hiding this comment

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

We cannot store nil time into time.Time variable.
Added description of the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

The zero value is time.Time{}, and can be checked like DeletedAt.IsZero()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, zero time can stand for null, but opposite doesn't work: null cannot stand for zero time when I scan values from DB:
panic: sql: Scan error on column index 2: unsupported Scan, storing driver.Value type <nil> into type *time.Time

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine; use a DB-related type in the DB code but don't export it into the rest of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use another variable to scan but I think it will complicate code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Putting an unfamiliar type in this struct is a huge overhead to understanding. Adding an extra variable in the scan code, which already has several variables for scanning only, is easy to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, PTAL

@lelenanam lelenanam requested a review from rndstr February 6, 2018 18:22
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cfg.DeletedAt has to be time or null to be able to store null time from DB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cfg.DeletedAt has to be time or null to be able to store null time from DB.

@lelenanam lelenanam changed the title Add deletedAt to config and rulesConfig (time or null) Scan deletedAt (time or null) in findConfigs, findRulesConfigs and GetConfig Feb 7, 2018
@bboreham bboreham merged commit ca432df into master Feb 8, 2018
@lelenanam lelenanam deleted the 1859-ruler branch July 21, 2018 04:59
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.

4 participants