HeartSaVioR commented on a change in pull request #26821:
[SPARK-20656][CORE]Support Incremental parsing of event logs in SHS
URL: https://github.com/apache/spark/pull/26821#discussion_r357013422
##########
File path:
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala
##########
@@ -981,14 +1011,28 @@ private[history] class FsHistoryProvider(conf:
SparkConf, clock: Clock)
logFiles: Seq[FileStatus],
replayBus: ReplayListenerBus,
maybeTruncated: Boolean,
+ info: Option[IncrimentalMetaInfo] = None,
eventsFilter: ReplayEventsFilter = SELECT_ALL_FILTER): Unit = {
// stop replaying next log files if ReplayListenerBus indicates some error
or halt
var continueReplay = true
+ var lineToSkip = info.map(_.lineToSkip).getOrElse(-1)
+ val fileToStart = info.map(_.fileIndex).getOrElse(0)
+ var fileIndex = 0
logFiles.foreach { file =>
- if (continueReplay) {
+ if (continueReplay && fileIndex >= fileToStart) {
Utils.tryWithResource(EventLogFileReader.openEventLog(file.getPath,
fs)) { in =>
- continueReplay = replayBus.replay(in, file.getPath.toString,
- maybeTruncated = maybeTruncated, eventsFilter = eventsFilter)
+ val result = replayBus.replay(in, file.getPath.toString,
+ maybeTruncated = maybeTruncated, eventsFilter = eventsFilter,
lineToSkip)
+ continueReplay = result.success
+ // We need to reset the lineToSkip to -1 as we need to parse next
file from the beginning
Review comment:
I'm not sure here is a good place to update the IncrementalMetaInfo:
1. The method doesn't know whether replaying events will be reflected in
app's KV store. I know `info` will be None for the case, but still not easy to
understand unless knowing full picture.
2. The logic seems to be broken when there's error being thrown. e.g. If
this reads 3 files successfully and throws error on reading 4th file, store may
reflect some events in 4th file (though I think it should be discarded), where
it doesn't update IncrementalMetaInfo to reflect the last status of store.
So it may be better to propagate the information to the caller and let
caller deal with it.
And as a side note this will not work with compaction (#26416) for various
reasons:
1) Listing log files will not start with index 0 if there's any compact file.
2) Log file for the index which should restart from may be removed (either
compacted into higher index of compact file, or equal index of compact file).
In either way, it should be replayed from the start (though compact log file
would be small enough to replay quickly).
But let's put aside of that - if #26416 becomes merged earlier it should be
reconsidered.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
