keboola / db-writer-snowflake

Snowflake database writer
MIT License
0 stars 1 forks source link

Disabled tables still get fetched #12

Open pivnicek opened 7 years ago

pivnicek commented 7 years ago

It looks like disabled tables in the writer aren't actually written out, but they are still fetched, and appear in the output table list.

ondrejhlavacek commented 7 years ago

That's a very good point. The UI creates the configuration with all input mappings and then disables the upload to Snowflake. This will happen in all writers. There's currently no way around it. Docker Runner executes the input mapping the way it is defined. Only the component itself will then know which tables to ignore. Currently there's no easy way around it.

pivnicek commented 7 years ago

IC, in that case I'll add it to PB.

Halama commented 7 years ago

I think this is a kind of bug and should not be in a PB. Maybe issue for docker runner.

Halama commented 7 years ago

but I have no idea how to fix it. Maybe either remove active/disabled states or docker runner should directly implement these flags in its IM.

pivnicek commented 7 years ago

I think it can maybe be handled/filtered by the input mapping class.

odinuv commented 7 years ago

to me the easiest seems to be to add support for disabled state in the input mapping loader

ondrejhlavacek commented 7 years ago

I don't see any point in having disabled input mapping. Either it's there and it's executed or not.

pivnicek commented 7 years ago

@odinuv handle it here?: https://github.com/keboola/docker-bundle/blob/master/Docker/Runner/DataLoader.php#L91

@ondrejhlavacek You may want to exclude a table from a job temporarily for some testing purpose? But fair point! lol

ondrejhlavacek commented 7 years ago

From the same point of view I don't really understand why would someone disable a table in a writer. Either it's there or not. When we have the functionality to duplicate configurations, it's super easy to clone a config, delete all unwanted tables and run it.

As a bonus it leaves easier to understand audit trail.

odinuv commented 7 years ago

but then you would have to delete the input mapping from configuration whent it gets disabled (and lose its configuration)

Halama commented 7 years ago

yes, for me that flag is an antipattern, something like comment code instead of delete it.

ondrejhlavacek commented 7 years ago

For testing purposes you'd want to execute the job for just one table. That's something we cannot do ATM.

odinuv commented 7 years ago

if we want to remove disabling of tables, we would have to ask the end-users what are the scenarios in which they use it

Halama commented 7 years ago

i think we were discussing this with some users when we were talking about simplified transformations.

ondrejhlavacek commented 7 years ago

We can get the writer configs that have disabled tables and let Jack go figure :-)

odinuv commented 7 years ago

figure what?

ondrejhlavacek commented 7 years ago

Why do they use it and if it can be accomplished in a better way, that does not seem like an antipattern.

odinuv commented 7 years ago

So we'll send Jack to some end-users using disabled tables so that he can come up with another solution? I would agree with that.

BTW, commented code is antipattern in production code, not everything in KBC is production code. (that's good reason for me personally)

ondrejhlavacek commented 7 years ago

Ad Jack: exactly.