You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "celinayk (via GitHub)" <gi...@apache.org> on 2025/10/14 10:24:02 UTC
[PR] [ZEPPELIN-6322] Filter download messages in ProcessData error stream [zeppelin]
celinayk opened a new pull request, #5104:
URL: https://github.com/apache/zeppelin/pull/5104
### What is this PR for?
This PR improves error stream output filtering in the `ProcessData` class to exclude download-related messages. Currently, Maven/npm download progress information clutters the error stream during integration tests, making it difficult to identify actual errors.
This change filters out download messages (e.g., "Downloading:", "Progress: 45%", "1024/2048 KB") while preserving real error messages.
### What type of PR is it?
Improvement
### Todos
* [x] - Add DOWNLOAD_PATTERNS array with 6 regex patterns
* [x] - Implement isDownloadMessage() method
* [x] - Apply filtering logic in buildOutputAndErrorStreamData()
### What is the Jira issue?
[ZEPPELIN-6322](https://issues.apache.org/jira/browse/ZEPPELIN-6322)
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6322] Filter download messages in ProcessData error stream [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5104:
URL: https://github.com/apache/zeppelin/pull/5104#issuecomment-5023906773
Merged into master
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6322] Filter download messages in ProcessData error stream [zeppelin]
Posted by "celinayk (via GitHub)" <gi...@apache.org>.
celinayk commented on PR #5104:
URL: https://github.com/apache/zeppelin/pull/5104#issuecomment-5022256211
> [ZEPPELIN-6322] Filter download messages in ProcessData error stream
Thanks for the thorough review, @tbonelee — really appreciate the depth here. Pushed a fix in c8225fabb that addresses all four points:
1. **Broad patterns swallowing real errors** — anchored all `DOWNLOAD_PATTERNS` with `^` so they only match at the start of a line, instead of matching anywhere via unanchored `.*`. `Task failed after writing
1024/2048 MB` and `Build failed at progress: 50%` no longer match at all.
2. **Downgrade instead of drop** — reordered exactly as you suggested: error/failed content is checked first and always goes to `LOGGER.warn(...)`, bypassing the download filter entirely. Lines that look like
download noise are routed to `LOGGER.trace(...)` instead of being dropped, so a misclassification can only make a line quieter, never lose it.
3. **Filtering ran on buffer chunks, not lines** — went a bit further than splitting `temp` on `\n`: added a `pendingErrorLine` buffer so a line split across two `BUFFER_LEN` reads (or even two outer-loop
iterations) gets reassembled into a complete line before classification. Any trailing unterminated content is flushed once the stream ends, so nothing at the tail gets silently swallowed either.
4. **Wording clarification** — added a comment above `sbErrorStream.append(tempSB)` noting that filtering only affects console log verbosity; `getErrorStream()` always returns the full, unfiltered content.
Also added `ProcessDataTest#doesNotFilterOutRealErrorLines()`, which includes both of your examples (`Task failed after writing 1024/2048 MB`, `Build failed at progress: 50%`) as regression cases, plus coverage for
the download-pattern matches and the chunk-boundary line-splitting scenario.
Let me know if anything still looks off
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6322] Filter download messages in ProcessData error stream [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee merged PR #5104:
URL: https://github.com/apache/zeppelin/pull/5104
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6322] Filter download messages in ProcessData error stream [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5104:
URL: https://github.com/apache/zeppelin/pull/5104#issuecomment-4946883542
Thanks for tackling this. The direction makes sense, and pulling the patterns into a constant array with an `isDownloadMessage()` helper reads cleanly.
I do want to raise one concern up front, because I think it should drive the whole design: **this change can silently hide real error messages, and in test output that's a costly failure mode.** When an integration test fails, these error lines are often the only clue we have. A filter that removes the wrong line doesn't just add noise, it can make a real failure look like it never happened, or send someone debugging in the wrong direction for hours. So I'd argue we should approach this as conservatively as possible: **decluttering is a nice-to-have, but never dropping a genuine error is a hard requirement.** A few points through that lens:
**1. The current patterns are broad enough to swallow real errors**
Several patterns match anywhere in a line, so they will catch legitimate messages that merely happen to contain a size or percentage figure, for example:
- `.*\d+/\d+\s*(KB|MB|GB|bytes).*` would match `Task failed after writing 1024/2048 MB`
- `.*progress:\s*\d+%.*` would match `Build failed at progress: 50%`
Worse, the download check runs before the `error`/`failed` check, so a matched line is discarded before we ever get a chance to recognize it as an error. That's exactly the case we can't afford.
**2. Prefer downgrading over dropping, so nothing is ever lost**
Given the above, I'd suggest we never fully drop a line. Routing suspected download lines to `LOGGER.trace()` instead of removing them keeps the default output just as clean, while guaranteeing a misfire can never erase a real error, only move it to a quieter log level:
```java
if (!temp.trim().isEmpty()) {
String lower = temp.toLowerCase();
if (lower.contains("error") || lower.contains("failed")) {
LOGGER.warn(temp.trim()); // real errors: always visible, never filtered
} else if (isDownloadMessage(temp)) {
LOGGER.trace(temp.trim()); // hidden by default, but recoverable
} else {
LOGGER.debug(temp.trim());
}
}
```
This ordering also guarantees anything containing `error`/`failed` bypasses the filter entirely. With this in place, a wrong pattern match becomes "logged at the wrong level" rather than "lost", which is the safety margin I think this change needs.
**3. The filter runs on buffer chunks, not lines**
Separately, `buildOutputAndErrorStreamData()` reads the error stream in fixed 300-char chunks (`BUFFER_LEN`), so `temp` usually holds several lines and partial lines, not one message. Since `Pattern.matches()` must match the entire input and `.*` doesn't cross `\n` without `DOTALL`, any chunk containing a newline matches nothing, so most real download output slips through unfiltered anyway. Splitting `temp` on `\n` and filtering per line would make it actually work, and would let the patterns be anchored with `^` to further shrink accidental matches.
**4. Minor: this affects console logging, not the returned error stream**
`sbErrorStream.append(tempSB)` runs before the filter, so `getErrorStream()` still returns the full text. That's fine if the intent is purely to declutter CI console logs, but since the title says "error stream" it may be worth clarifying the wording or the intended target.
Once the approach settles, a unit test on `isDownloadMessage()` would be valuable, especially a case with a real error line that contains a size/percentage figure, since that's the exact scenario we're trying not to regress on.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6322] Filter download messages in ProcessData error stream [zeppelin]
Posted by "celinayk (via GitHub)" <gi...@apache.org>.
celinayk closed pull request #5104: [ZEPPELIN-6322] Filter download messages in ProcessData error stream
URL: https://github.com/apache/zeppelin/pull/5104
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org