[b/356461225] Add HDFS progress logging #534
Conversation
dawidxc
requested review from
paolomorandini,
mjquinn-google,
arotenberg-google and
shevek-google
as code owners
.../main/java/com/google/edwmigration/dumper/application/dumper/connector/hdfs/ScanContext.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Could this use https://github.com/google/dwh-migration-tools/blob/main/dumper/lib-common/src/main/java/com/google/edwmigration/dumper/plugin/ext/jdk/progress/ConcurrentRecordProgressMonitor.java instead, particularly if the 'total' field was updated to be a mutable LongAdder? If we want an ETA, I think adding that to ConcurrentProgressMonitor would be a lovely feature update.
Note that System.currentTimeMillis() is a very expensive call, which does not show up in a stack-sampling profiler, because it's not a safepoint; BlockProgressMonitor is designed to reduce that overhead.
| .append("\nTotal time: ") | ||
| .append(timeSinceScanBegin.getSeconds() + "s") | ||
| .append("\nTotal time in listStatus(..): ") | ||
| .append(timeSpentInListStatus.getSeconds() + "s") |
There was a problem hiding this comment.
This should probably be a chained append(), not a String-+ in the middle, as that creates an extra StringBuilder and String.
| lastLogTime = now; | ||
| long percentFinished = | ||
| Longs.constrainToRange(this.numDirsWalked * 100 / totalDirectoryCount, 0, 99); | ||
| partialProgressLogger.logProgress("HDFS scan " + percentFinished + "% completed"); |
There was a problem hiding this comment.
A lot of this looks very similar to the shared/common code in [Concurrent]RecordProgressMonitor. Would it be possible to use that?
No description provided.