amaechler opened a new pull request, #20381:
URL: https://github.com/apache/druid/pull/20381
### Description
`EnvironmentVariableDynamicConfigProvider.getConfig()` resolves every
configured variable with `System.getenv` and puts the result into the map
unconditionally. `System.getenv` returns `null` for a variable that is not set,
so an unset variable surfaces as a `null` value in the resolved config. Every
consumer of `DynamicConfigProvider` copies that map into something that doesn't
support `null` values, so one unset variable fails the whole component.
With this PR, `getConfig()` now puts only entries whose value resolved to
non-null, and logs a skipped variable at info level with its name and key
(never a value):
```
INFO ... EnvironmentVariableDynamicConfigProvider - Environment variable
[KAFKA_CLIENT_RACK] for key [client.rack] is not set, skipping.
```
I also added a comment to `DynamicConfigProvider.getConfig()` to state that
only entries with non-null values are returned
(`MapStringDynamicConfigProvider` already does the same, since its
`ImmutableMap` cannot hold null values).
<details>
#### Background
I was looking int a change to set Kafka's `client.rack` per pod from an
environment variable, so consumers fetch from the closest replica. Not every
pod has that variable. A supervisor spec like this:
```json
"consumerProperties": {
"bootstrap.servers": "...",
"druid.dynamic.config.provider": {
"type": "environment",
"variables": { "client.rack": "KAFKA_CLIENT_RACK" }
}
}
```
fails the supervisor and every ingestion task on a pod without
`KAFKA_CLIENT_RACK`:
```
java.lang.NullPointerException
at java.util.Properties.setProperty(Properties.java:231)
at
org.apache.druid.indexing.kafka.KafkaRecordSupplier.addConsumerPropertiesFromConfig(KafkaRecordSupplier.java:337)
```
An absent `client.rack` is Kafka's default, so the spec should simply have
worked. The only workaround is to inject an always-empty `KAFKA_CLIENT_RACK`
into every pod.
#### What this does for each consumer
All consumers copy the resolved map at construction time. None of them
checks for `null`, and none uses `containsKey` to detect configuration, so an
absent key is safe everywhere.
*
[`KafkaRecordSupplier.addConsumerPropertiesFromConfig`](https://github.com/apache/druid/blob/3cbd00d9d7dc175acf3964d8bde0c07ab95a2624/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java#L331-L339)
copies with `properties.setProperty(e.getKey(), e.getValue())`. `Properties`
throws `NullPointerException` on a null value. This is the supervisor and task
failure above. It now gets the key omitted and Kafka applies its default.
*
[`KafkaEmitter`](https://github.com/apache/druid/blob/3cbd00d9d7dc175acf3964d8bde0c07ab95a2624/extensions-contrib/kafka-emitter/src/main/java/org/apache/druid/emitter/kafka/KafkaEmitter.java#L131)
does `props.putAll(config.getKafkaProducerSecrets().getConfig())` into a
`Properties`. Same `NullPointerException`, now at emitter startup. Same fix.
*
[`DynamicConfigProviderUtils`](https://github.com/apache/druid/blob/3cbd00d9d7dc175acf3964d8bde0c07ab95a2624/processing/src/main/java/org/apache/druid/utils/DynamicConfigProviderUtils.java#L32-L44)
does `newConfig.putAll(dynamicConfig)` into a `HashMap`, which accepts null.
The null then flows into the Confluent schema registry client
(`SchemaRegistryBasedAvroBytesDecoder`,
`SchemaRegistryBasedProtobufBytesDecoder`) and the Iceberg catalogs
(`GlueIcebergCatalog`, `HiveIcebergCatalog`, `RestIcebergCatalog`), where
behaviour depends on the library. Hadoop's `Configuration.set`, for example,
rejects a null value with an `IllegalArgumentException`. These consumers now
get a consistent absent key instead.
*
[`RabbitStreamRecordSupplier`](https://github.com/apache/druid/blob/3cbd00d9d7dc175acf3964d8bde0c07ab95a2624/extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamRecordSupplier.java#L129-L136)
iterates entries and assigns `username` and `password` fields directly. An
unset variable used to assign `null`, now it leaves the field untouched.
#### Alternatives considered
* **Fix each consumer instead.** There are nine call sites using three
different copy idioms. The provider is the single producer of the null, so it
is the one place to fix. A consumer also cannot tell "variable unset" from
"provider bug".
* **Fail fast with a descriptive error.** Whether a key is required is
consumer knowledge. The provider cannot know that `client.rack` is optional and
`ssl.keystore.password` is not. Failing fast would also not fix the case above.
* **A configuration flag for strict or lenient behaviour.** Nobody wants the
`NullPointerException` branch, and a bug fix should not add config surface.
* **Log level.** A misspelled variable name is now skipped instead of
crashing, so the skip is logged with the variable and key. Info rather than
warn, because an absent variable can be intentional, as in the rack example.
Resolution runs once per supervisor start, task start, sampler request or
emitter construction, so the volume is one line per component.
* **Empty string stays a value.** A variable set to `""` is put as `""`. For
`client.rack` that equals Kafka's default, so explicitly clearing a key remains
possible.
Only specs that fail outright today change behaviour. A spec whose variables
are all set resolves exactly as before.
</details>
#### Release note
The `environment` dynamic config provider now omits variables that are not
set in the environment instead of producing a `null` value that fails consumers
such as the Kafka supervisor with a `NullPointerException`. A skipped variable
is logged at info level.
<hr>
##### Key changed/added classes in this PR
* `EnvironmentVariableDynamicConfigProvider`
* `DynamicConfigProvider`
* `EnvironmentVariableDynamicConfigProviderTest`
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] a release note entry in the PR description.
- [x] added Javadocs for most classes and all non-trivial methods. Linked
related entities via Javadoc links.
- [x] added comments explaining the "why" and the intent of the code
wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths,
ensuring the threshold for [code
coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md)
is met.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
