You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/01/27 21:34:17 UTC
[GitHub] [spark] viirya opened a new pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override individual StateStoreCustomMetric
viirya opened a new pull request #31369:
URL: https://github.com/apache/spark/pull/31369
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This patch proposes to sum up custom metric values instead of taking arbitrary one when combining `StateStoreMetrics`.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
For stateful join in structured streaming, we need to combine `StateStoreMetrics` from both left and right side. Currently we simply take arbitrary one from custom metrics with same name from left and right. By doing this we miss half of metric number.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Yes, this corrects metrics collected for stateful join.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Unit test.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769445986
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134624/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566970583
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreMetricsSuite.scala
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.streaming.state
+
+import org.apache.spark.SparkFunSuite
+
+class StateStoreMetricsSuite extends SparkFunSuite {
+
+ test("StateStoreMetrics.combine should not override individual metrics") {
Review comment:
Can we have this in `StateStoreSuite.scala` because this is only one and `StateStoreMetrics` is in `StateStore.scala`?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566977044
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreMetricsSuite.scala
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.streaming.state
+
+import org.apache.spark.SparkFunSuite
+
+class StateStoreMetricsSuite extends SparkFunSuite {
+
+ test("StateStoreMetrics.combine should not override individual metrics") {
+ val customMetric = StateStoreCustomSumMetric("metric1", "custom metric 1")
Review comment:
Do you think we can have an extended test case including `StateStoreCustomSizeMetric` and `StateStoreCustomTimingMetric`?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566977383
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
+ val sameMetrics = customMetrics.filter(_._1 == customMetric)
+ val sumOfMetrics = sameMetrics.map(_._2).sum
Review comment:
yes
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-774639158
@xuanyuanking Thanks for continuing looking this.
> Here's some analysis:
The left/rightSideMetrics passed in combine function are objects with adding new descriptions here:
Oh, yeah, looks like `SymmetricHashJoinStateManager.metrics` changes metric description to prevent the metrics to be overridden. However I'd say we still should make `StateStoreMetrics.combine` solid to prevent possible metric overriding as the added test case shows.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769999470
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39248/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xuanyuanking commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
xuanyuanking commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-774634462
Sorry for pinging you again on this issue. I touched this code today and have a question:
@viirya Could you provide which metric missed half of the result in your scenario? It seems that the overridden shouldn't happen in the original codebase for the custom metrics framework. Do you use some special way for adding the custom metrics?
Here's some analysis:
The left/rightSideMetrics passed in `combine` function are objects with adding new descriptions here:
https://github.com/apache/spark/blob/e3058ba17cb4512537953eb4ded884e24ee93ba2/sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/SymmetricHashJoinStateManager.scala#L316-L321
So the result of `combine` should contain two metrics object with the same name. They will finally get summed in https://github.com/apache/spark/blob/e3058ba17cb4512537953eb4ded884e24ee93ba2/sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/StreamingSymmetricHashJoinExec.scala#L403-L405
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566304481
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val customMetricNames = allMetrics.flatMap(_.customMetrics.keySet).map(_.name).toSet
Review comment:
Something like this?
```scala
val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
val customMetrics = allMetrics.flatMap(_.customMetrics)
val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
val sameMetrics = customMetrics.filter(_ == customMetric)
val sumOfMetrics = sameMetrics.map(_._2).sum
customMetric -> sumOfMetrics
}.toMap
```
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566308008
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val customMetricNames = allMetrics.flatMap(_.customMetrics.keySet).map(_.name).toSet
Review comment:
Hmm, although the some custom metrics like `StateStoreCustomSumMetric` are case class, the base trait `StateStoreCustomMetric` can also be implemented in class too. So currently comparing ` StateStoreCustomMetric` directly looks okay, I'm not sure if it is possible to fail if someone implements a custom metric with class, not case class.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya edited a comment on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya edited a comment on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-774639158
@xuanyuanking Thanks for continuing looking this.
> Here's some analysis:
The left/rightSideMetrics passed in combine function are objects with adding new descriptions here:
Oh, yeah, looks like `SymmetricHashJoinStateManager.metrics` changes metric description to prevent the metrics to be overridden. However I'd say we still should make `StateStoreMetrics.combine` solid to prevent possible metric overriding as the added test case shows.
> It seems that the overridden shouldn't happen in the original codebase for the custom metrics framework.
It possibly happens. I think we should not rely on the caller of `StateStoreMetrics.combine` to change metric name/description to avoid overridding metrics.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-768624118
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39157/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566972173
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreMetricsSuite.scala
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.streaming.state
+
+import org.apache.spark.SparkFunSuite
+
+class StateStoreMetricsSuite extends SparkFunSuite {
+
+ test("StateStoreMetrics.combine should not override individual metrics") {
Review comment:
ok
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-770160257
Merged to master/3.1/3.0/2.4.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769980719
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39248/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r568291799
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
Review comment:
Hm? You mean when there is only one custom metric without duplication? I think doing sum does not get incorrect result. Is there any benefit?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-770008082
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566537278
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val customMetricNames = allMetrics.flatMap(_.customMetrics.keySet).map(_.name).toSet
Review comment:
Maybe you can override `equal()` for the base trait if you have such concern.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-768753423
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134569/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-770079221
**[Test build #134661 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134661/testReport)** for PR 31369 at commit [`f950772`](https://github.com/apache/spark/commit/f9507725d86c3da4bb799b987761eb340a708c8e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xuanyuanking commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
xuanyuanking commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r568288897
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
Review comment:
nit but might be necessary: should we only do the sum logic for distinctCustomMetrics only when the overwriting happens? If you agree I'll submit a follow-up PR and let you review. @viirya @HeartSaVioR
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
Review comment:
```
I think we don't blindly call this method with one element. No benefit then.
```
Make sense.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-768620032
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39157/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-768740599
**[Test build #134569 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134569/testReport)** for PR 31369 at commit [`d019731`](https://github.com/apache/spark/commit/d019731db7c3af9b42446f30ac9602e1f4c8e144).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-768592776
**[Test build #134569 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134569/testReport)** for PR 31369 at commit [`d019731`](https://github.com/apache/spark/commit/d019731db7c3af9b42446f30ac9602e1f4c8e144).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-774929505
Hopefully, callers in the future could be always aware that `StateStoreMetrics` is combined by `StateStoreCustomMetric` rather than `StateStoreCustomMetric.name`. Otherwise, usages like
```scala
combinedMetrics.customMetrics.foreach { case (metric, value) =>
longMetric(metric.name) = value
^ "=" instead of "+="
}
```
could result in the wrong metrics again.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566983774
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreSuite.scala
##########
@@ -978,6 +978,28 @@ abstract class StateStoreSuiteBase[ProviderClass <: StateStoreProvider]
assert(store.metrics.memoryUsedBytes > noDataMemoryUsed)
}
+ test("SPARK-34270: StateStoreMetrics.combine should not override individual metrics") {
+ val customMetric = StateStoreCustomSumMetric("metric1", "custom metric 1")
Review comment:
Thank you for updating. Maybe, `customMetric` -> `customSumMetric`?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769953174
**[Test build #134661 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134661/testReport)** for PR 31369 at commit [`f950772`](https://github.com/apache/spark/commit/f9507725d86c3da4bb799b987761eb340a708c8e).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566976528
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
+ val sameMetrics = customMetrics.filter(_._1 == customMetric)
+ val sumOfMetrics = sameMetrics.map(_._2).sum
Review comment:
Just a question. Is it valid for all classes using `StateStoreCustomMetric` trait? At least for the following three in Spark.
```
case class StateStoreCustomSumMetric(name: String, desc: String) extends StateStoreCustomMetric
case class StateStoreCustomSizeMetric(name: String, desc: String) extends StateStoreCustomMetric
case class StateStoreCustomTimingMetric(name: String, desc: String) extends StateStoreCustomMetric
```
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566978102
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreMetricsSuite.scala
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.streaming.state
+
+import org.apache.spark.SparkFunSuite
+
+class StateStoreMetricsSuite extends SparkFunSuite {
+
+ test("StateStoreMetrics.combine should not override individual metrics") {
+ val customMetric = StateStoreCustomSumMetric("metric1", "custom metric 1")
Review comment:
okay
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769952597
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134664/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769370157
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39212/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r568293221
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
Review comment:
So if you assume the chance allMetrics is only having one element, then all these calculations are unnecessary. But this method already assumes the case we want multiple metrics to be combined, and I think we don't blindly call this method with one element. No benefit then.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-768753423
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134569/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769983228
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39251/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xuanyuanking commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
xuanyuanking commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r568326258
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
Review comment:
```
I think we don't blindly call this method with one element. No benefit then.
```
Make sense.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769297293
**[Test build #134624 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134624/testReport)** for PR 31369 at commit [`9ac7253`](https://github.com/apache/spark/commit/9ac7253d682b94b9f8cbce70c58b841935d39be4).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r568291356
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
Review comment:
My understanding is that combine is only called with multiple elements, where we expect these elements will have same custom metrics (as these are from state store provider). Please correct me if I'm missing here.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769948516
Thank you @dongjoon-hyun
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769445986
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134624/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-770087726
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134661/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-770087726
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134661/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769952597
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134664/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r565945169
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val customMetricNames = allMetrics.flatMap(_.customMetrics.keySet).map(_.name).toSet
Review comment:
nit: why not use `StateStoreCustomMetric` to filter the same metric directly?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769333460
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39212/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-770008083
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r568291799
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
Review comment:
Hm? You mean when there is only one custom metric without duplication? I think doing sum does not get incorrect result. Is there any benefit?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769953174
**[Test build #134661 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134661/testReport)** for PR 31369 at commit [`f950772`](https://github.com/apache/spark/commit/f9507725d86c3da4bb799b987761eb340a708c8e).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r568291356
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
Review comment:
My understanding is that combine is only called with multiple elements, where we expect these elements will have same custom metrics (as these are from state store provider). Please correct me if I'm missing here.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
Review comment:
So if you assume the chance allMetrics is only having one element, then all these calculations are unnecessary. But this method already assumes the case we want multiple metrics to be combined, and I think we don't blindly call this method with one element. No benefit then.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769315008
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39212/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun closed pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #31369:
URL: https://github.com/apache/spark/pull/31369
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566970583
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreMetricsSuite.scala
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.streaming.state
+
+import org.apache.spark.SparkFunSuite
+
+class StateStoreMetricsSuite extends SparkFunSuite {
+
+ test("StateStoreMetrics.combine should not override individual metrics") {
Review comment:
Can we have this in `StateStoreSuite.scala` because this is only one and `StateStoreMetrics` and `StateStoreCustomSumMetric` is in `StateStore.scala`?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override individual StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-768592776
**[Test build #134569 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134569/testReport)** for PR 31369 at commit [`d019731`](https://github.com/apache/spark/commit/d019731db7c3af9b42446f30ac9602e1f4c8e144).
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r566986384
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreSuite.scala
##########
@@ -978,6 +978,28 @@ abstract class StateStoreSuiteBase[ProviderClass <: StateStoreProvider]
assert(store.metrics.memoryUsedBytes > noDataMemoryUsed)
}
+ test("SPARK-34270: StateStoreMetrics.combine should not override individual metrics") {
+ val customMetric = StateStoreCustomSumMetric("metric1", "custom metric 1")
Review comment:
Changed.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xuanyuanking commented on a change in pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
xuanyuanking commented on a change in pull request #31369:
URL: https://github.com/apache/spark/pull/31369#discussion_r568288897
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala
##########
@@ -165,10 +165,18 @@ case class StateStoreMetrics(
object StateStoreMetrics {
def combine(allMetrics: Seq[StateStoreMetrics]): StateStoreMetrics = {
+ val distinctCustomMetrics = allMetrics.flatMap(_.customMetrics.keys).distinct
+ val customMetrics = allMetrics.flatMap(_.customMetrics)
+ val combinedCustomMetrics = distinctCustomMetrics.map { customMetric =>
Review comment:
nit but might be necessary: should we only do the sum logic for distinctCustomMetrics only when the overwriting happens? If you agree I'll submit a follow-up PR and let you review. @viirya @HeartSaVioR
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-768624132
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39157/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-768624132
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39157/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-768871997
cc. @tdas @zsxwing @jose-torres @gaborgsomogyi @xuanyuanking to see some chance of more eyes to review.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769992452
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39251/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769370157
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/39212/
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on pull request #31369: [SPARK-34270][SS] Combine StateStoreMetrics should not override StateStoreCustomMetric
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #31369:
URL: https://github.com/apache/spark/pull/31369#issuecomment-769530196
LGTM
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org