HyukjinKwon commented on code in PR #59327:
URL: https://github.com/apache/spark/pull/59327#discussion_r4240452706
##########
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala:
##########
@@ -1772,6 +1761,60 @@ private[history] class FsHistoryProvider(conf:
SparkConf, clock: Clock)
private[spark] object FsHistoryProvider {
+ /**
+ * Positions `in` at the start of the first line that begins at or after
byte `target`,
Review Comment:
**Nit (P3):** `in` itself isn't left at the line start. When a `\n` is
found, `in` has already been read up to 8 KiB past the boundary
(`in.read(buffer)`), and only the returned `SequenceInputStream(rest, in)`
starts at the line. "Bytes read past the boundary are put back in front of the
stream" is also ambiguous, because they are prepended only in the returned
stream, not pushed back into `in`.
The only caller uses the returned stream, but a future caller who trusts
this sentence and keeps reading `in` (for example the `tryWithResource` handle)
would silently lose up to 8 KiB of events. Something like this would be
clearer: "Returns a stream that starts at the first line beginning at or after
byte `target` of `in`, together with that line's byte offset. `in` itself is
consumed and must not be read afterwards; bytes read past the boundary are
prepended to the returned stream."
**Verification:**
- **Inspection:** The scaladoc no longer claims that `in` itself is
positioned at the line start, and its description of the returned stream, the
consumption of `in` and the non-positive-target case matches the implementation.
##########
core/src/test/scala/org/apache/spark/deploy/history/FsHistoryProviderSuite.scala:
##########
@@ -1248,6 +1248,142 @@ abstract class FsHistoryProviderSuite extends
SparkFunSuite with Matchers with P
}
}
+ test("end event reparse when the skip offset lands inside a multi-byte
character") {
+ val log = newLogFile("end-event-utf8", None, inProgress = false)
+ val bytes = writeRawLog(log, endEventTestLines("end-event-utf8",
+ jobStartLine(1, 0x4e2d.toChar.toString * 200)))
+ // The job description is written as raw UTF-8: skip to the second byte of
a character.
+ val leadByte = bytes.indexWhere(b => (b & 0xF0) == 0xE0)
+ assert(leadByte > 0)
+ val conf = createTestConf().set(END_EVENT_REPARSE_CHUNK_SIZE,
(bytes.length - leadByte - 1L))
Review Comment:
**Nit (P3):** With `bytes.length - leadByte - 1`, `target` is the second
byte of U+4E2D (E4 B8 AD), but `skipToLineStart` now skips only `target - 1`
bytes, so the raw scan starts on the lead byte E4, a character boundary. The
test still catches the original bug (skipping exactly `target` bytes and then
decoding). However, it would also pass with an alignment that decodes UTF-8
from `target - 1` before looking for `\n`, and that alignment fails whenever
`target - 1` is a continuation byte, which is exactly the case the raw-byte
scan is for.
Using `bytes.length - leadByte - 2` puts the target on the third byte, so
the scan starts on B8. With that change and an updated comment, this test would
cover a mid-character start. A couple of multi-byte cases in the
`skipToLineStart` unit test, with the target on each byte of a character, would
also pin this down at the helper level.
**Verification:**
- **Regression:** A completed log whose reparse offset falls inside a
multi-byte character, with the raw scan starting on a continuation byte, is
listed as completed, and a decode-before-scan alignment would make the test
fail.
- **Behavior:** skipToLineStart returns the first line starting at or after
the target, and its UTF-8 byte offset, for targets on the lead, second and
third byte of a 3-byte character.
--
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]
