-
Notifications
You must be signed in to change notification settings - Fork 748
[GOBBLIN-2186] Emit GoT GTEs to time WorkUnit prep and to record volume of Work Discovery
#4089
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -43,6 +43,7 @@ | |||||
| import org.apache.gobblin.converter.initializer.ConverterInitializerFactory; | ||||||
| import org.apache.gobblin.destination.DestinationDatasetHandlerService; | ||||||
| import org.apache.gobblin.metrics.event.EventSubmitter; | ||||||
| import org.apache.gobblin.metrics.event.TimingEvent; | ||||||
| import org.apache.gobblin.runtime.AbstractJobLauncher; | ||||||
| import org.apache.gobblin.runtime.JobState; | ||||||
| import org.apache.gobblin.runtime.troubleshooter.AutomaticTroubleshooter; | ||||||
|
|
@@ -60,6 +61,8 @@ | |||||
| import org.apache.gobblin.temporal.ddm.work.WorkUnitsSizeSummary; | ||||||
| import org.apache.gobblin.temporal.ddm.work.assistance.Help; | ||||||
| import org.apache.gobblin.temporal.workflows.metrics.EventSubmitterContext; | ||||||
| import org.apache.gobblin.temporal.workflows.metrics.EventTimer; | ||||||
| import org.apache.gobblin.temporal.workflows.metrics.TemporalEventTimer; | ||||||
| import org.apache.gobblin.writer.initializer.WriterInitializerFactory; | ||||||
|
|
||||||
|
|
||||||
|
|
@@ -127,6 +130,9 @@ public GenerateWorkUnitsResult generateWorkUnits(Properties jobProps, EventSubmi | |||||
| int numSizeSummaryQuantiles = getConfiguredNumSizeSummaryQuantiles(jobState); | ||||||
| WorkUnitsSizeSummary wuSizeSummary = digestWorkUnitsSize(workUnits).asSizeSummary(numSizeSummaryQuantiles); | ||||||
| log.info("Discovered WorkUnits: {}", wuSizeSummary); | ||||||
| // IMPORTANT: send prior to `writeWorkUnits`, so the volume of work discovered (and bin packed) gets durably measured. even if serialization were to | ||||||
| // exceed available memory and this activity execution were to fail, a subsequent re-attempt would know the amount of work, to guide re-config/attempt | ||||||
| createWorkPreparedSizeDistillationTimer(wuSizeSummary, eventSubmitterContext).stop(); | ||||||
|
|
||||||
| JobStateUtils.writeWorkUnits(workUnits, workDirRoot, jobState, fs); | ||||||
| JobStateUtils.writeJobState(jobState, workDirRoot, fs); // ATTENTION: the writing of `JobState` after all WUs signifies WU gen+serialization now complete | ||||||
|
|
@@ -150,26 +156,28 @@ public GenerateWorkUnitsResult generateWorkUnits(Properties jobProps, EventSubmi | |||||
| protected List<WorkUnit> generateWorkUnitsForJobStateAndCollectCleanupPaths(JobState jobState, EventSubmitterContext eventSubmitterContext, Closer closer, | ||||||
| Set<String> pathsToCleanUp) | ||||||
| throws ReflectiveOperationException { | ||||||
| // report (timer) metrics for "Work Discovery", *planning only* - NOT including WU prep, like serialization, `DestinationDatasetHandlerService`ing, etc. | ||||||
| // IMPORTANT: for accurate timing, SEPARATELY emit `.createWorkPreparationTimer`, to record time prior to measuring the WU size required for that one | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean here one GTE event for serialization step or GTE event for everything discovery serialization and all ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. originally, in gobblin/gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java Line 476 in 7dbeebf
that is what's included in the the timer for WU prep happens a bit later - gobblin/gobblin-runtime/src/main/java/org/apache/gobblin/runtime/AbstractJobLauncher.java Line 549 in 7dbeebf
so in this comment:
I just meant that we're timing only planning/creation, not the preparation such as serialization. as for WU serialization, there is no existing, historical event strictly for that. typically that only takes a long time when memory-constrained and GC-bound. although we could consider adding a new event to time that, for purposes of right-sizing, GC stats are more interesting than the duration it happens to take. if anything, the former is what I'd prioritize. |
||||||
| TemporalEventTimer.Factory timerFactory = new TemporalEventTimer.WithinActivityFactory(eventSubmitterContext); | ||||||
| EventTimer workDiscoveryTimer = timerFactory.createWorkDiscoveryTimer(); | ||||||
| Source<?, ?> source = JobStateUtils.createSource(jobState); | ||||||
| WorkUnitStream workUnitStream = source instanceof WorkUnitStreamSource | ||||||
| ? ((WorkUnitStreamSource) source).getWorkunitStream(jobState) | ||||||
| : new BasicWorkUnitStream.Builder(source.getWorkunits(jobState)).build(); | ||||||
|
|
||||||
| // TODO: report (timer) metrics for workunits creation | ||||||
| if (workUnitStream == null || workUnitStream.getWorkUnits() == null) { // indicates a problem getting the WUs | ||||||
| String errMsg = "Failure in getting work units for job " + jobState.getJobId(); | ||||||
| log.error(errMsg); | ||||||
| // TODO: decide whether a non-retryable failure is too severe... (in most circumstances, it's likely what we want) | ||||||
| // TODO: decide whether a non-retryable failure is too severe... (some sources may merit retry) | ||||||
| throw ApplicationFailure.newNonRetryableFailure(errMsg, "Failure: Source.getWorkUnits()"); | ||||||
| } | ||||||
| workDiscoveryTimer.stop(); | ||||||
|
|
||||||
| if (!workUnitStream.getWorkUnits().hasNext()) { // no work unit to run: entirely normal result (not a failure) | ||||||
| log.warn("No work units created for job " + jobState.getJobId()); | ||||||
| return Lists.newArrayList(); | ||||||
| } | ||||||
|
|
||||||
| // TODO: count total bytes for progress tracking! | ||||||
|
|
||||||
| boolean canCleanUpTempDirs = false; // unlike `AbstractJobLauncher` running the job end-to-end, this is Work Discovery only, so WAY TOO SOON for cleanup | ||||||
| DestinationDatasetHandlerService datasetHandlerService = closer.register( | ||||||
| new DestinationDatasetHandlerService(jobState, canCleanUpTempDirs, eventSubmitterContext.create())); | ||||||
|
|
@@ -264,6 +272,19 @@ protected static WorkUnitsSizeDigest digestWorkUnitsSize(List<WorkUnit> workUnit | |||||
| return new WorkUnitsSizeDigest(totalSize.get(), topLevelWorkUnitsDigest, constituentWorkUnitsDigest); | ||||||
| } | ||||||
|
|
||||||
| protected static EventTimer createWorkPreparedSizeDistillationTimer( | ||||||
| WorkUnitsSizeSummary wuSizeSummary, EventSubmitterContext eventSubmitterContext) { | ||||||
| // Inspired by a pair of log messages produced within `CopySource::getWorkUnits`: | ||||||
| // 1. Statistics for ConcurrentBoundedPriorityIterable: {ResourcePool: {softBound: [ ... ], hardBound: [ ...]},totalResourcesUsed: [ ... ], \ | ||||||
| // maxRequirementPerDimension: [entities: 231943.0, bytesCopied: 1.22419622769628E14], ... } | ||||||
| // 2. org.apache.gobblin.data.management.copy.CopySource - Bin packed work units. Initial work units: 27252, packed work units: 13175, \ | ||||||
| // max weight per bin: 500000000, max work units per bin: 100. | ||||||
| // rather than merely logging, durably emit this info, to inform re-config for any potential re-attempt (should WU serialization OOM) | ||||||
| TemporalEventTimer.Factory timerFactory = new TemporalEventTimer.WithinActivityFactory(eventSubmitterContext); | ||||||
| return timerFactory.createWorkPreparationTimer() | ||||||
| .withMetadataAsJson(TimingEvent.WORKUNITS_GENERATED_SUMMARY, wuSizeSummary.distill()); | ||||||
| } | ||||||
|
|
||||||
| public static int getConfiguredNumSizeSummaryQuantiles(State state) { | ||||||
| return state.getPropAsInt(GenerateWorkUnits.NUM_WORK_UNITS_SIZE_SUMMARY_QUANTILES, GenerateWorkUnits.DEFAULT_NUM_WORK_UNITS_SIZE_SUMMARY_QUANTILES); | ||||||
| } | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, if serialization takes time and if it accounts for more OOM issue then should we consider these both in separate activity and launched through one parent workflow as IIUC activity retry will do everything from beginning and discovered workunits will be generated again which can also lead to duplication of GTE, whats your opinion on this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having WU planning separate from WU serialization is worth considering. that would allow for a re-attempt of only serialization w/o needing to rerun the planning. there's no concern in sending the GTE again on the re-attempt - that's not the motivation. instead the benefit would be to expedite the re-attempt by subsequently skipping a repeat of successful WU planning.
to separate into two activities we'd need to succeed in persisting some intermediate form of WU planning, so the WU planning activity could "pass input" to the WU serialization activity, as the two won't execute together - or possibly even on the same host. that intermediate form clearly ought to be more likely to succeed than serializing all the WUs themselves - the failure we're trying to address.
this major design choice awaits if we decide to pursue such larger rework.