Improved getPropertiesBySource() Output -- tech-support #216 #188

vnandwana · 2025-10-23T19:40:04Z

Improved property source resolution and logged conflicts
Consolidated sources into hierarchy of precedence: EnvironmentVariables > RuntimeConfiguration > KillBillDefaults
Added status indicators and conflict warnings

Ticket -- https://github.com/killbill/technical-support/issues/216

Sample output: getPropertiesBySource-Output.log

* Consolidated sources into hierarchy of precedence: EnvironmentVariables > RuntimeConfiguration > KillBillDefaults * Added status indicators and conflict warnings

sbrossie

Which items of the list from https://github.com/killbill/technical-support/issues/216 does it address - all of them? I.e. Does it cover for Verify (and fix as needed) if we have config from killbill-commons, killbill-platform - e.g. PersistentQueueConfig ?.Please be clear about this PR covers and what it does not cover.
Regarding logic about precedence, this is what I would expect:

Kill Bill defaults - e.g. user.timezone # Kill Bill overrides some properties regardless of config
Env variables
System properties - e.g. killbill.properties or -D`
Default Kill Bill - e.g. *Config files
[ENV, RUNTIME PROPERTIES, ], could you first 1/ document the expected behavior, 2/ write unit tests to cover all the cases, and 3/ adjust the implementation to match expected behavior.

Can you write unit tests and verify that new code works as expected?

sbrossie · 2025-10-27T19:25:45Z

base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java

+
+                        final List<String> overridden = getOverriddenSources(source, sources, sourceOrder);
+                        if (!overridden.isEmpty()) {
+                            displayValue = effectiveValue + " [ACTIVE -- overrides: " + String.join(", ", overridden) + "]";


Am I understanding correctly that value will now be a concatenated string of the real value + a status using [...]? Does it mean client needs to parse this string to extract raw value?

This does not work, either we need to change the type of the sourceProperties to track both value and some indication about override and which one is active OR (probably simpler) we do not return this info and simply have a WARN.

* Removed property conflict string from property values. * Added a unit test to verify getPropertiesBySource() behavior and source precedence.

vnandwana · 2025-10-30T20:14:55Z

@sbrossie, addressed the review comments. Please take a look at your convenience and share the feedback. Thank you.

We can override system default properties except user.timezone. Documented here -- killbill/killbill-docs#632

These changes address the top 4 subtasks mentioned in the ticket.

Sample output:
getPropertiesBySource-Output-Latest.log

sbrossie

I am not super thrilled about the following:
• The code keeps different structures, i.e. properties (Properties) and propertiesCollector (PropertiesWithSourceCollector) -> Shouldn't the properties be the values of propertiesCollector ?
• The 2 apis rely on Properties getProperties(); and Map<String, Map<String, String>> getPropertiesBySource() rely on 2 different mechanisms - how are we sure that result is consistent across?
• The computation of the result from the APIs above should be done once - and not every time we call the method

sbrossie · 2025-11-04T04:14:34Z

base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java

        final Map<String, String> configuration = new HashMap<>();
-        configuration.put("org.killbill.dao.user", "root");
-        configuration.put("org.killbill.dao.password", "password");
+        configuration.put("org.killbill.server.shutdownDelay", "1s");


Are those used in the test - if so, which source should they be part of?

sbrossie · 2025-11-04T04:15:06Z

base/src/test/java/org/killbill/billing/platform/config/TestDefaultKillbillConfigSource.java

    }

    @Test(groups = "fast")
    public void testGetPropertiesBySource() throws URISyntaxException, IOException {


Is there a way to test the env properties as well?

…getPropertiesBySource(). * Cached computed results for improved performance. * Added unit tests.

vnandwana · 2025-11-10T17:29:07Z

Latest output of getProperties() and getPropertiesBySource() methods:

getProperties()-Output.log
getPropertiesBySource()-Output.log

sbrossie

From a functionality aspect this seem to work well, but see my comment about properties. Also, I am seeing too many WARN:

2025-11-11T18:54:26,511+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.persistent.bus.external.inMemory': defined in sources [RuntimeConfiguration, PersistentBusConfig] - using value from 'RuntimeConfiguration': 'true'
2025-11-11T18:54:26,512+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.invoice.maxDailyNumberOfItemsSafetyBound': defined in sources [RuntimeConfiguration, InvoiceConfig] - using value from 'RuntimeConfiguration': '-1'
2025-11-11T18:54:26,512+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.notificationq.main.claimed': defined in sources [RuntimeConfiguration, NotificationQueueConfig] - using value from 'RuntimeConfiguration': '1'
2025-11-11T18:54:26,512+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.server.multitenant': defined in sources [RuntimeConfiguration, KillbillServerConfig] - using value from 'RuntimeConfiguration': 'true'
2025-11-11T18:54:26,513+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.billing.osgi.dao.password': defined in sources [RuntimeConfiguration, OSGIDataSourceConfig] - using value from 'RuntimeConfiguration': 'root'
2025-11-11T18:54:26,513+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.persistent.bus.main.queue.mode': defined in sources [RuntimeConfiguration, PersistentBusConfig] - using value from 'RuntimeConfiguration': 'STICKY_EVENTS'

-> Any override of 'KillBillDefaults' config should not come as a WARN as this expected. WARN should be resrved for use case where we override using different schemes, i.e. 'EnvironmentVariables' and 'RuntimeConfiguration'...

sbrossie · 2025-11-11T19:03:13Z

base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java

+                                                       "KillBillDefaults"));

+    private final PropertiesWithSourceCollector propertiesCollector;
    private final Properties properties;


Is there a good reason to still have to maintain this set of properties - I would expect all APIs to rely on the propertiesCollector instead to avoid having to parallel state.

It has been addressed in this commit.

This commit does have some sys out statements (didn't mean to push this commit).

* Implemented lazy cache initialization in getPropertiesBySource() * Added getPropertyDirect() to avoid circular dependency during initialization * Simplified getString() to use cached properties after initialization * Improved addProperties logic in PropertiesWithSourceCollector * Fixed JNDI lookup to explicitly dereference Reference objects

vnandwana · 2025-11-20T15:51:51Z

From a functionality aspect this seem to work well, but see my comment about properties. Also, I am seeing too many WARN:

2025-11-11T18:54:26,511+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.persistent.bus.external.inMemory': defined in sources [RuntimeConfiguration, PersistentBusConfig] - using value from 'RuntimeConfiguration': 'true'
2025-11-11T18:54:26,512+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.invoice.maxDailyNumberOfItemsSafetyBound': defined in sources [RuntimeConfiguration, InvoiceConfig] - using value from 'RuntimeConfiguration': '-1'
2025-11-11T18:54:26,512+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.notificationq.main.claimed': defined in sources [RuntimeConfiguration, NotificationQueueConfig] - using value from 'RuntimeConfiguration': '1'
2025-11-11T18:54:26,512+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.server.multitenant': defined in sources [RuntimeConfiguration, KillbillServerConfig] - using value from 'RuntimeConfiguration': 'true'
2025-11-11T18:54:26,513+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.billing.osgi.dao.password': defined in sources [RuntimeConfiguration, OSGIDataSourceConfig] - using value from 'RuntimeConfiguration': 'root'
2025-11-11T18:54:26,513+0000 lvl='WARN', log='DefaultKillbillConfigSource', th='main', xff='', rId='', tok='', aRId='', tRId='', Property conflict detected for 'org.killbill.persistent.bus.main.queue.mode': defined in sources [RuntimeConfiguration, PersistentBusConfig] - using value from 'RuntimeConfiguration': 'STICKY_EVENTS'

-> Any override of 'KillBillDefaults' config should not come as a WARN as this expected. WARN should be resrved for use case where we override using different schemes, i.e. 'EnvironmentVariables' and 'RuntimeConfiguration'...

This has been fixed in this commit.

vnandwana · 2025-11-20T16:22:39Z

Latest output of getProperties() and getPropertiesBySource() methods:

getProperties()-Output.log getPropertiesBySource()-Output.log

Output still looks the same.

sbrossie · 2025-11-20T18:50:52Z

base/src/main/java/org/killbill/billing/platform/config/DefaultKillbillConfigSource.java

        return key.replace(ENVIRONMENT_VARIABLE_PREFIX, "").replaceAll("_", "\\.");
    }

+    private String getPropertyDirect(final String propertyName) {


Is this really necessary? Why don't we simply compute the cachedPropertiesBySource from the CTOR and use those to implement all our APIs, i.e. OSGIConfigProperties#getProperties(), OSGIConfigProperties#getPropertiesBySource().

Is this to support the use cases for our tests that may add dynamically properties using the DefaultKillbillConfigSource#setProperty api?

Yes, it's to handle dynamic updates via setProperty(). Also getString() is called within the constructor (we need to rebuild cache multiple times) and *Config classes register with RuntimeConfigRegistry after DefaultKillbillConfigSource construction completes, so cache built in constructor miss *Config classes.

* Improved property source resolution and logged conflicts

46c58dc

* Consolidated sources into hierarchy of precedence: EnvironmentVariables > RuntimeConfiguration > KillBillDefaults * Added status indicators and conflict warnings

vnandwana requested review from pierre and sbrossie October 23, 2025 19:56

sbrossie requested changes Oct 27, 2025

View reviewed changes

* Added ImmutableSystemProperties to properties collector.

ee3ac64

* Removed property conflict string from property values. * Added a unit test to verify getPropertiesBySource() behavior and source precedence.

vnandwana requested a review from sbrossie October 30, 2025 20:06

Fixed property resolution order

bd22b4c

sbrossie reviewed Nov 4, 2025

View reviewed changes

* Refactored to use a single source of truth for getProperties() and …

ead5cc0

…getPropertiesBySource(). * Cached computed results for improved performance. * Added unit tests.

* Fixed formatting.

d8c818c

vnandwana requested a review from sbrossie November 10, 2025 19:07

sbrossie requested changes Nov 11, 2025

View reviewed changes

sbrossie mentioned this pull request Nov 11, 2025

Improved config value collection and instance name replacement handling killbill/killbill-commons#189

Merged

Added debug msg to TestJNDIManager

e4613cd

vnandwana marked this pull request as draft November 20, 2025 12:20

vnandwana marked this pull request as ready for review November 20, 2025 15:29

vnandwana requested a review from sbrossie November 20, 2025 16:32

sbrossie approved these changes Nov 20, 2025

View reviewed changes

sbrossie merged commit a8f5d77 into killbill:master Nov 20, 2025
10 checks passed

Uh oh!

Improved getPropertiesBySource() Output -- tech-support #216 #188

Improved getPropertiesBySource() Output -- tech-support #216 #188

Uh oh!

Conversation

vnandwana commented Oct 23, 2025

Uh oh!

sbrossie left a comment

Choose a reason for hiding this comment

Uh oh!

sbrossie Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

vnandwana commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbrossie left a comment

Choose a reason for hiding this comment

Uh oh!

sbrossie Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

sbrossie Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

vnandwana commented Nov 10, 2025

Uh oh!

sbrossie left a comment

Choose a reason for hiding this comment

Uh oh!

sbrossie Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

vnandwana Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

vnandwana commented Nov 20, 2025

Uh oh!

vnandwana commented Nov 20, 2025

Uh oh!

sbrossie Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

vnandwana Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vnandwana commented Oct 30, 2025 •

edited

Loading