DanielLeens commented on PR #11738:
URL: https://github.com/apache/seatunnel/pull/11738#issuecomment-5279183236
@nzw921rx You're right, and thanks for catching this — my earlier approval
missed it.
I traced the actual runtime behavior to confirm:
- `PrometheusSinkOptions.BATCH_SIZE` declares
`defaultValue(DEFAULT_BATCH_SIZE)` where `DEFAULT_BATCH_SIZE = 1024`.
- `PrometheusSinkConfig` declares `private int batchSize;` with no explicit
initializer, so its Java default is `0`.
- `ReadonlyConfig.getOptional(...)` returns `Optional.empty()` when the key
is genuinely absent from user config — it does NOT fall back to the option's
declared default (only `ReadonlyConfig.get(...)` does that, via
`getOptional(option).orElseGet(option::defaultValue)`).
So in `PrometheusSinkConfig.loadConfig`, the current code:
```java
if (pluginConfig.getOptional(PrometheusSinkOptions.BATCH_SIZE).isPresent()) {
sinkConfig.setBatchSize(pluginConfig.get(PrometheusSinkOptions.BATCH_SIZE));
}
```
skips the assignment entirely when `batch_size` is omitted, leaving
`sinkConfig.batchSize` at `0`, not the documented `1024`.
That's not just cosmetic — `PrometheusWriter.write(Point)` guards size-based
flushing with `if (batchSize > 0 && batchList.size() >= batchSize)`. With
`batchSize == 0`, that condition is always false, so size-triggered flushing is
silently disabled whenever a user omits `batch_size` (the common case), and
rows only get flushed on the `flush_interval` timer (default 300000 ms = 5
min). That's an unbounded in-memory buffer for up to 5 minutes on any
decent-throughput job, which is a real behavioral divergence from the
documented default.
Your simplification is the correct fix:
```java
sinkConfig.setBatchSize(pluginConfig.get(PrometheusSinkOptions.BATCH_SIZE));
```
This makes `get()` apply `orElseGet(option::defaultValue)` so the declared
`1024` default is actually honored, while `OptionRule`'s
`Conditions.greaterThan(BATCH_SIZE, 0)` (added in this PR) still validates any
explicitly-supplied value.
Note this bug pre-dates this PR (the old `checkArgument` path had the same
`isPresent()` guard), so it isn't something introduced by this change — but
since we're already touching this exact block, I'd treat it as a small blocking
fix rather than a follow-up: could you push one more commit applying the
one-line simplification above? Everything else in this PR (the
`OptionRule`/`Conditions` migration itself) still looks correct to me.
--
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]
