Certain Druid property overrides are ignored due to dynamically added properties

[maltesander]

Affected version

All

Current and expected behavior

We have some dynamically resolved druid properties that are added in the ConfigMap after the static and override merging has happened via product config.

This will lead to ignoring several (and not documented) properties that therefore cannot be overridden anymore.

Expected

All properties are over-ridable

Possible solution

The way this is implemented, we should first collect all dynamically added properties in a Map which then is extended by the merged static and config overrides properties (currently it is the other way around).

Additional context

This came up in a discussion https://github.com/orgs/stackabletech/discussions/10

[fhennig]

I've investigated.

The transform_all_roles_to_config function is applying the overrides already. It takes the role configs and spits out the <String, String> map. It calls the compute_files function on the DruidCluster first, and in our original design it was intended that all computed properties are computed in that function.

It turns out that this was not possible, because we need to resolve things first (like the LDAP authentication class) which are not available in the compute_files function.

So we do changes to the config after the overrides are already applied, further down the line. I think this is a bit of a design flaw in our original design and in the long run I think we should correct this.

Most likely, all operators are affected.

I also found this comment in the code:

Add any properties derived from storage manifests, such as segment cache locations.
This has to be done here since there is no other suitable place for it.
Previously such properties were added in the compute_files() function,
but that code path is now incompatible with the design of fragment merging.

As a short term fix I think we could set these properties only if they are not already set.

[fhennig]

We want to do a quick fix now

After the quick fix we want to tackle the underlying issue

[fhennig]

@maltesander what do you think about testing, to prevent this in the future? Do we want to add a test for this? Nor sure how we could, because we'd have to test overrides for all properties. This would be nice as a unit test actually

[maltesander]

It would be nice to have tests for this. I would add this to one of the integration tests though. Unit testing this will be hard without refactoring / mocking.

[fhennig]

I've talked to Malte, we will not test this for now, until we make some refactorings that will make testing easier. integration tests for this are overkill.