kennknowles commented on code in PR #32953:
URL: https://github.com/apache/beam/pull/32953#discussion_r2153095892
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/status/MemoryMonitor.java:
##########
@@ -573,6 +621,7 @@ private static File getLoggingDir() {
public File dumpHeap()
throws MalformedObjectNameException, InstanceNotFoundException,
ReflectionException,
MBeanException, IOException {
+ Preconditions.checkState(canDumpHeap);
Review Comment:
An error message here would be good, so if it ever happens the user will
report it. Maybe something like "Bug! Attempt to dump heap even though it
should be disabled."
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/status/MemoryMonitor.java:
##########
@@ -194,21 +196,35 @@ public long totalGCTimeMilliseconds() {
private final File localDumpFolder;
+ private final boolean gzipCompress;
+
public static MemoryMonitor fromOptions(PipelineOptions options) {
SdkHarnessOptions sdkHarnessOptions = options.as(SdkHarnessOptions.class);
- String uploadFilePath = options.getTempLocation();
+ @Nullable String uploadFilePath =
sdkHarnessOptions.getRemoteHeapDumpLocation();
+ if (uploadFilePath == null) {
+ uploadFilePath = options.getTempLocation();
Review Comment:
Would be useful to have defensive tests of this code path. (aka a couple
tests of `fromOptions` with different options set)
##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/status/MemoryMonitor.java:
##########
@@ -237,14 +255,26 @@ private MemoryMonitor(
boolean canDumpHeap,
double gcThrashingPercentagePerPeriod,
@Nullable String uploadFilePath,
- File localDumpFolder) {
+ File localDumpFolder,
+ boolean gzipCompress) {
this.gcStatsProvider = gcStatsProvider;
this.sleepTimeMillis = sleepTimeMillis;
this.shutDownAfterNumGCThrashing = shutDownAfterNumGCThrashing;
- this.canDumpHeap = canDumpHeap;
this.gcThrashingPercentagePerPeriod = gcThrashingPercentagePerPeriod;
this.uploadFilePath = uploadFilePath;
this.localDumpFolder = localDumpFolder;
+ this.gzipCompress = gzipCompress;
+
+ if (canDumpHeap) {
+ try {
+ Files.createDirectories(localDumpFolder.toPath());
Review Comment:
I don't think we should be doing this kind of logic in the constructor.
Perhaps in the static factory method, or lazily at heap dump time?
--
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: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
