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

add a timestamp column reflecting when record is synced #156

Open
Jclevinson opened this issue Sep 10, 2020 · 10 comments
Open

add a timestamp column reflecting when record is synced #156

Jclevinson opened this issue Sep 10, 2020 · 10 comments

Comments

@Jclevinson
Copy link

Jclevinson commented Sep 10, 2020

Feature requested by Aurum users (NDOH project in RSA). They've chosen to add this as a column to the tables in to Azure db but enquired if our tool had this feature; Cory and others on DET chat seem to agree this sounds like a good idea.

To be clear there is no urgent client-facing need for adding this as they've solved it on their side for the time being. However they hinted that for audit purposes they'd want to see it from ours. Will update if prio changes.

@czue
Copy link
Member

czue commented Sep 11, 2020

@snopoke @millerdev and others, I'd be curious to hear what you think the right spec for this is. I think there's not really any paradigm for system-defined columns in the spec at the moment.

My first thought would be to create a global namespace for "source field" to map to system properties. E.g. something like:

_.inserted_at

(note the underscore).

Another option would be to handle this in the "map via" column (e.g. have a new map via that is insertion-date or something). This would, I guess, just ignore the source field column(?) which feels a little weird.

@snopoke
Copy link
Contributor

snopoke commented Sep 11, 2020

I was thinking that we could make it part of the SqlTableWriter to always add the column and value rather than having the user specify it in the config. The downside is that it would add it to every existing table.

@millerdev
Copy link
Contributor

It seems possible to achieve this without adding new features for the DET: add an extra column to your table that has a default value of the current time.

However, I can see an argument for why that is too complicated for some folks. The trade-off is that the DET becomes more complicated, which is also not ideal. I like the idea of adding a global system property namespace rather than automatically adding the column to every table.

@Jclevinson
Copy link
Author

Jclevinson commented Sep 21, 2020

Hey all, client changed their mind, and has come back requesting this to be added to the sync tool as soon as feasible. What do you think is the outlook on a solution and timeline? @czue not sure what the right pathway is for a product request like this during your offline time; let me know.

@kaapstorm FYI

@czue
Copy link
Member

czue commented Sep 23, 2020

@Jclevinson let's take that conversation to email. I'll send a follow up.

@czue
Copy link
Member

czue commented Sep 23, 2020

@millerdev @snopoke any further thoughts on the best way to do this? another possible option could be a command line argument - e.g. --with-timestamps or something. That would prevent the issue of having to add it to every table, though I guess the downside is that it could lead to a pollution of command line args if more of these come up...

Of the four proposals mentioned I think my rank order is:

  1. Namespaced system property in the config file.
  2. --with-timestamps CLI arg
  3. Automatically add it
  4. new map_via type

(though don't feel too strongly about any of them)

@czue
Copy link
Member

czue commented Sep 23, 2020

for options 2 and 3 what happens if the default column name conflicts with an explicit column? I guess we just fail hard? That might be problematic for existing exports that upgrade the DET and then break...

@snopoke
Copy link
Contributor

snopoke commented Sep 23, 2020

On further consideration I concur with you that option 1 is the best option since it won't impact existing setups and users can control the column name.

@millerdev
Copy link
Contributor

I agree that option 1 is the best if the feature is required.

I would add another option: do not add this feature. I would rank that nearly equivalent to option 1. I will admit that I do not have a clear understanding of the problem that the people requesting this feature are facing, so I do not have proper empathy for their pain. I'm interested to hear if/why it is not "possible to achieve this without adding new features for the DET?"

@czue
Copy link
Member

czue commented Sep 24, 2020

@millerdev I think the main problem, framed as a user story, is "I don't really understand the internals of databases and I want this information available in my analysis using the config file/format and process I"m used to."

If you set it up in the DB you have to figure out how to get it added it to every table, and how to make sure it gets re-added if you blow away tables or change them. Also while it's easy in postgres, in other DBs like Azure it's more complex. Whereas if you use the DET to manage it then all of that is handled for you and you don't need to do any manual fiddling with SQL

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

No branches or pull requests

4 participants