eon2208 opened a new pull request, #25339:
URL: https://github.com/apache/flink/pull/25339
## What is the purpose of the change
* Fixes incorrect Ports display in the PrometheusReporter constructor in
case of the httpServer creation failure. Instead of printing the port numbers,
the error message currently displays an Iterator object reference, which is not
useful for debugging.
## Brief change log
* Collected the port numbers into a list (attemptedPorts) while attempting
to create the HTTP server, and used this list in the error message if server
creation fails. This ensures that the actual ports are printed in the exception
message instead of the Iterator reference.
## Verifying this change
- Extends the existing cannotStartTwoReportersOnSamePort test by checking
the exception message thrown to verify that the log message contains the actual
ports and not the object reference.
- Examples
- before: `Could not start PrometheusReporter HTTP server on any
configured port. Ports: org.apache.flink.util.UnionIterator@67065fd3`
- after: `Could not start PrometheusReporter HTTP server on any
configured port. Ports: [9000, 9001, 9002, ...]`
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with
`@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its
components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
### TBD
I have also considered changing the constructor parameter from
`Iterable<Integer>` to `List<Integer>`, as it would provide a more
straightforward way to manage the port numbers. This change would require minor
adjustments in the codebase, namely modifying the `PrometheusReporterFactory`
and some test cases, which currently use `Iterable<Integer>` and would need
conversion to `List<Integer>`. I’d like to discuss if this change is
worthwhile, as it could improve code clarity but requires slightly more effort.
Alternatively, converting the `Iterable<Integer>` to a `List<Integer>` at
the beginning of the constructor, and then attempting to create the HTTP server
using this list, could also be a solution. In my opinion, this would make the
code a bit cleaner and easier to read. However, it would require iterating over
the ports twice: once when converting the `Iterable` to a list, and again
during the actual HTTP server initialization. While this extra iteration may
have a small impact on performance, I don't think it would be significant
enough to cause issues.
**Questions for Discussion**:
- Is it worth refactoring the constructor to use `List<Integer>` instead of
`Iterable<Integer>`, considering the minor adjustments required across the
codebase?
- Would the conversion from `Iterable` to `List` in the constructor be a
good trade-off for improved readability, even if it introduces a second
iteration?
- Does the current implementation fit well as-is, or could these refinements
be valuable improvements?
--
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]
