You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/06/24 12:57:36 UTC
[GitHub] [beam] iemejia commented on a change in pull request #11956: [BEAM-8133] Publishing results of Nexmark tests to InfluxDB
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:
users@infra.apache.org