GitHub user ppkarwasz created a discussion: Triage of the 75 stale open PRs
We currently have 76 open PRs: #470 and 75 PRs dating from 2014 to 2023.
I asked an LLM to review the remaining PRs to see which could still be useful
and which should be closed. Summary:
- **24 PRs are still useful**, the problem they address is still present on
trunk, though most need a rebase or partial rework.
- **45 PRs should be closed here**, they target components that moved to a
satellite repository, are already fixed, or are superseded. A few of the
moved-component fixes may still apply in their new home (I did not verify them
against the satellite repos), so the close note can invite authors to re-open
there.
- **6 PRs need a maintainer decision** (project direction or design questions).
## Still useful after adaptation (24)
### Taildir source and file channel bug fixes (11)
This is the most valuable cluster. All of these bugs are still present on
current trunk:
| PR | Fix | Current state on trunk |
|---|---|---|
| #336 | Rename/rotation duplication, opt-in `inodeOnly` tracking |
`ReliableTaildirEventReader` still reopens a renamed file at position 0 (path
check in `updateTailFiles`) |
| #130 | Same rename bug, unconditional, includes a regression test | Same
code, needs rebase onto trunk's newer guards |
| #295 | File handle leak and duplication when an inode reappears under a new
path | The old `RandomAccessFile` is still never closed before replacement |
| #368 | TOCTOU between `getInode()` and `lastModified()`/`length()` resets
position to 0 on rotation | Attribute reads are still non-atomic |
| #369 | Races between `process()`, the position writer and the idle checker
threads | `existingInodes` is cleared while iterated, `tailFiles` is a plain
`HashMap` |
| #313 | Position file truncated (left empty) on write failure, losing all
positions | `new FileWriter(file)` still truncates before writing; most of the
diff is whitespace noise, needs cleanup |
| #367 | Spurious ERROR on startup when the position file is empty | Symptom of
#313, trivial guard |
| #298 | Open file handles kept until `idleTimeout` after rotation or deletion
| Real, but the fix needs hardening (`NoSuchFileException` aborts the checker) |
| #279 | Unbounded duplicate growth of `idleInodes` when the channel is full |
One-line List to Set change, already had an approval |
| #290 | Dead `List<File> result;` variable in `TaildirMatcher` | Still
present, trivial |
| #173 | File channel: `int` overflow sizing the checkpoint file for capacity
above ~268M events | One-character `(long)` cast, still applies cleanly |
If we consolidate the rename/duplication/leak family, #336 plus the close logic
of #295 plus the atomic attribute read of #368 cover the whole cluster, and
#313 (with #367) covers position-file durability.
### Core, SDK, scripts and packaging (13)
| PR | What it does | Caveat |
|---|---|---|
| #410 | `JSONEvent.setBody` uses the platform default charset | Applies
cleanly today |
| #292 | Rollback failure in `ChannelProcessor` suppresses the original
exception | Rebase only |
| #204 | Remove `CounterGroup` from `StressSource` | Had an approval already,
minor rebase |
| #326 | ZooKeeper-based config misses the env-var interpolation file/URI
configs get | Better reimplemented as a `MapResolver.resolveProperties` call |
| #216 | `bin/flume-ng` clobbers caller-set `FLUME_CLASSPATH`, `JAVA_OPTS`,
etc. | Small `${VAR:-default}` fix, still applies |
| #135 | Byte-unit suffixes (k/m/g) for capacity settings | Rebase onto
`Context`'s new home in `flume-ng-sdk` |
| #32 | Size-based rolling for `RollingFileSink` | The `FIXME: Rotate on size`
is still in the code; `PathManager` became an interface since |
| #269 | `RollingFileSink` should create or check `sink.directory` | Idea
valid, patch as written would NPE, reimplement |
| #411 | Fix flaky `testMinimumRequiredSpaceTooSmallforPut` | Cause confirmed,
but the fix leaks a stream, rework |
| #351 | Official Docker image | Dockerfile part adaptable; CI and test parts
must be redone for the logging-parent workflows |
| #272 | `flume.root.logger`-style overrides in `conf/log4j2.xml` | Only the
`log4j2.xml` hunk survives, and it has an unclosed-brace bug |
| #140 | Fix `sink.event.drain.sucess` counter typo | The string is a published
JMX attribute name, so this is a breaking change for dashboards |
| #18 | Extension-friendly HTTP source | Gap still exists, but the patch
targets Jetty 6, needs a full Jetty 12 rework |
## Proposed for closing (45)
### Component moved to a satellite repository (35)
These PRs target code that no longer lives in this repository. They should be
closed here; where noted, the fix may still be relevant in the component's new
home and the author can be invited to re-open there.
- **HDFS sink**, now `flume-hdfs-sink` in
[apache/logging-flume-hadoop](https://github.com/apache/logging-flume-hadoop):
#103, #104, #110, #114, #115, #180, #223, #250, #256, #345
- **Hive sink**, now `flume-hive-sink` in
[apache/logging-flume-hadoop](https://github.com/apache/logging-flume-hadoop):
#206, #315
- **HBase sink**, superseded by `flume-hbase2-sink` in
[apache/logging-flume-hadoop](https://github.com/apache/logging-flume-hadoop)
(the HBase 1 sink these PRs target was dropped): #20, #30 (the one-character
javadoc typo from #20, `IllegalArguemntException`, still exists in
`flume-ng-sdk` `Context` and can be fixed directly)
- **Kafka source/sink/channel**, now in
[apache/logging-flume-kafka](https://github.com/apache/logging-flume-kafka):
#65, #93, #96, #280, #417
- **Spooling directory source and deserializers**, now under `flume-rpc-avro`
in [apache/logging-flume-rpc](https://github.com/apache/logging-flume-rpc):
#38, #41, #76, #99, #193
- **Scribe source**, now in
[apache/logging-flume-scribe](https://github.com/apache/logging-flume-scribe):
#218, #219
- **Twitter source**, now in
[apache/logging-flume-twitter](https://github.com/apache/logging-flume-twitter):
#11, #123
- **Thrift source**, now under `flume-rpc-thrift` in
[apache/logging-flume-rpc](https://github.com/apache/logging-flume-rpc): #102
- **JMS source**, now in
[apache/logging-flume-jms](https://github.com/apache/logging-flume-jms): #405
- **Elasticsearch/Twitter tests**: #12 (the touched tests now live in
[apache/logging-flume-search](https://github.com/apache/logging-flume-search)
and
[apache/logging-flume-twitter](https://github.com/apache/logging-flume-twitter);
the remaining locale issue here, `TestRateLimiter`, was fixed separately in
#469)
- **Other moved or removed code:** #317 (`NettyAvroRpcClient` javadoc; the
class is now in `flume-rpc-avro` in
[apache/logging-flume-rpc](https://github.com/apache/logging-flume-rpc)), #45
(syslog UDP 2K limit; the source stayed here but was rewritten on Netty 4,
which already grows the buffer to 64K), #192 (`ResettableFileInputStream`
stayed in `flume-ng-sdk` but its only consumer, the spooling directory reader,
moved to logging-flume-rpc), #176 (new redis channel; new integrations belong
in satellite repositories, not here)
### Superseded or already fixed (6)
- #291, #167: superseded by #336/#130 (same rename bug, weaker fixes; #167 does
not compile)
- #14: a delimiter-split interceptor; the surviving `RegexExtractorInterceptor`
already extracts body substrings into headers, and the motivating use case
(partitioning HDFS sink paths) moved out with the HDFS sink
- #145: the `ChannelSelectorConfiguration` comparison bug is already fixed on
trunk
- #10: superseded by URI-based configuration (`--conf-uri`)
- #82: superseded by the config-filter framework (`ExternalProcessConfigFilter`
covers the use case); was explicitly WIP
### Wrong or not a bug (4)
- #278: fixes a `Sink.process()` contract violation no in-tree sink triggers,
and the fix would busy-loop
- #293: swaps header key and value, deletes the mandatory-parameter validation
- #112: user-supplied JVM options already win by last-occurrence ordering in
`run_flume`
- #43: personal debug comments plus an unconfigurable behavior change to
roll-file naming
## Needs a maintainer decision (6)
- #136 (Kinesis sink) and #133 (Pub/Sub source): with the split into satellite
repositories, new cloud integrations would belong in a new `logging-flume-*`
repository rather than here; both diffs are 2017-era and would need a full
rework either way.
- #217 (ZooKeeper agent registration) and #220 (DynamicMBean ad-hoc counters):
the target modules still exist, but these are large additions with no in-tree
consumer, written against heavily rewritten code.
- #207 (taildir emits the last line of a file with no trailing newline) and
#159 (write taildir positions on channel commit to reduce CPU): both address
real issues, but the proposed designs are problematic (emitting lines a writer
has not finished, coupling SDK events to taildir). They need a design
discussion, or a close with an invitation to re-propose.
## Side findings
While checking the PRs, two leftovers from the module moves turned up on trunk:
- `src/site/antora/modules/ROOT/pages/FlumeUserGuide.adoc` still documents the
HDFS Sink and references the Kafka channel, both of which moved to their
satellite repositories.
- The `IllegalArguemntException` javadoc typo in `flume-ng-sdk` `Context` (from
#20 above).
## Proposed next steps
1. Close the 45 PRs above with a short note pointing to this discussion; for
the moved components, the note should name the satellite repository and invite
the author to re-open there if the fix still applies.
2. Treat the 11 taildir/file-channel fixes as a consolidated effort rather than
11 separate rebases.
3. Decide on the 6 judgment calls.
4. Clean up the stale user-guide sections.
GitHub link: https://github.com/apache/logging-flume/discussions/474
----
This is an automatically sent email for [email protected].
To unsubscribe, please send an email to: [email protected]
