iemejia commented on a change in pull request #11956:
URL: https://github.com/apache/beam/pull/11956#discussion_r444759631
##########
File path:
sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/Main.java
##########
@@ -142,21 +148,27 @@ void runAll(String[] args) throws IOException {
saveSummary(null, configurations, actual, baseline, start, options);
}
- if (options.getExportSummaryToBigQuery()) {
- ImmutableMap<String, String> schema =
- ImmutableMap.<String, String>builder()
- .put("timestamp", "timestamp")
- .put("runtimeSec", "float")
- .put("eventsPerSec", "float")
- .put("numResults", "integer")
- .build();
+ final ImmutableMap<String, String> schema =
+ ImmutableMap.<String, String>builder()
+ .put("timestamp", "timestamp")
+ .put("runtimeSec", "float")
+ .put("eventsPerSec", "float")
Review comment:
Do we use this one? it looks with runtimeMs + numResults this is not
needed anymore or we can deduce it if someone cares.
##########
File path:
sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/Main.java
##########
@@ -142,21 +148,27 @@ void runAll(String[] args) throws IOException {
saveSummary(null, configurations, actual, baseline, start, options);
}
- if (options.getExportSummaryToBigQuery()) {
- ImmutableMap<String, String> schema =
- ImmutableMap.<String, String>builder()
- .put("timestamp", "timestamp")
- .put("runtimeSec", "float")
- .put("eventsPerSec", "float")
- .put("numResults", "integer")
- .build();
+ final ImmutableMap<String, String> schema =
+ ImmutableMap.<String, String>builder()
+ .put("timestamp", "timestamp")
+ .put("runtimeSec", "float")
Review comment:
Since the goal is to improve the existing use case can we make this an
integer and use ms instead to make it more precise?
##########
File path:
sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/NexmarkQueryName.java
##########
@@ -42,8 +42,8 @@
PROCESSING_TIME_WINDOWS(12), // Query "12"
// Other non-numbered queries
- BOUNDED_SIDE_INPUT_JOIN,
- SESSION_SIDE_INPUT_JOIN;
+ BOUNDED_SIDE_INPUT_JOIN(13),
Review comment:
:+1:
##########
File path:
sdks/java/testing/test-utils/src/main/java/org/apache/beam/sdk/testutils/publishing/InfluxDBPublisher.java
##########
@@ -19,14 +19,19 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.joining;
+import static
org.apache.beam.repackaged.core.org.apache.commons.lang3.StringUtils.isBlank;
Review comment:
Do not depend on repackaged commons-lang3 this will probably be removed
in the future so better add the explicit commons-lang3 import and corresponding
classes.
##########
File path:
sdks/java/testing/test-utils/src/main/java/org/apache/beam/sdk/testutils/publishing/InfluxDBPublisher.java
##########
@@ -66,30 +125,43 @@ private static void publish(
builder.setDefaultCredentialsProvider(provider);
}
- final HttpPost postRequest = new HttpPost(settings.host + "/write?db=" +
settings.database);
+ return builder;
+ }
- final StringBuilder metricBuilder = new StringBuilder();
- results.stream()
Review comment:
nit: The original code with the strings looks uglier but somehow is
easier to understand in a single read (so easier to maintain), the new one
requires a lot of methods and jumping back and forth in code for not much. Can
we go back to the older approach
##########
File path: .test-infra/metrics/kubernetes/beam-influxdb.yaml
##########
@@ -24,6 +24,7 @@ metadata:
data:
init-script.iql: |
CREATE RETENTION POLICY "a_year" ON "beam_test_metrics" DURATION 52w
REPLICATION 1 DEFAULT
+ CREATE RETENTION POLICY "forever" ON "beam_test_metrics" DURATION INF
REPLICATION 1
Review comment:
<3
##########
File path:
sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/Main.java
##########
@@ -142,21 +148,27 @@ void runAll(String[] args) throws IOException {
saveSummary(null, configurations, actual, baseline, start, options);
}
- if (options.getExportSummaryToBigQuery()) {
- ImmutableMap<String, String> schema =
- ImmutableMap.<String, String>builder()
- .put("timestamp", "timestamp")
- .put("runtimeSec", "float")
- .put("eventsPerSec", "float")
- .put("numResults", "integer")
- .build();
+ final ImmutableMap<String, String> schema =
+ ImmutableMap.<String, String>builder()
+ .put("timestamp", "timestamp")
+ .put("runtimeSec", "float")
+ .put("eventsPerSec", "float")
+ .put("numResults", "integer")
+ .build();
+ if (options.getExportSummaryToBigQuery()) {
savePerfsToBigQuery(
BigQueryResultsPublisher.create(options.getBigQueryDataset(),
schema),
options,
actual,
start);
}
+
+ if (options.getExportSummaryToInfluxDB()) {
+ final long timestamp = start.getMillis() / 1000; // seconds
Review comment:
Oh I thought timestamps in Influxe were in ms well probably we don't
need that level of precision for the start timestamp.
----------------------------------------------------------------
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:
us...@infra.apache.org
