HeartSaVioR commented on code in PR #49816:
URL: https://github.com/apache/spark/pull/49816#discussion_r1957492421
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStore.scala:
##########
@@ -321,6 +321,35 @@ case class StateStoreCustomTimingMetric(name: String,
desc: String) extends Stat
SQLMetrics.createTimingMetric(sparkContext, desc)
}
+case class StateStoreInstanceMetric(
Review Comment:
I don't like the fact we inherit StateStoreCustomMetric while we don't want
to handle this like the same with StateStoreCustomMetric, hence we are going to
check the type and apply the logic differently.
I'd rather just isolate two and introduce StateStoreInstanceMetric as a same
level with StateStoreCustomMetric.
I propose to introduce a "trait" (a.k.a interface) about
StateStoreInstanceMetric which does not inherit StateStoreCustomMetric, and put
the logic to handle this trait in different methods than where we are handling
StateStoreCustomMetric. The trait should open for createSQLMetric to be
implemented from the derived class, as well as how to compare two instance of
the derived class (a.k.a Ordering).
```
trait StateStoreInstanceMetric {
def metricPrefix: String
def descPrefix: String
def partitionId: Option[Int] = None
def storeName: String = StateStoreId.DEFAULT_STORE_NAME
override def createSQLMetric(sparkContext: SparkContext): SQLMetric
/** This should follow the convention of Comparable.compareTo() on the
return value. */
def compareTo(other: StateStoreInstanceMetric): Int
/** When this instance ever has an update. */
def isNonZero: Boolean
override def name: String = {
assert(partitionId.isDefined, "Partition ID must be defined for instance
metric name")
s"$metricPrefix.partition_${partitionId.get}_${storeName}"
}
override def desc: String = {
assert(partitionId.isDefined, "Partition ID must be defined for instance
metric description")
s"$descPrefix (partitionId = ${partitionId.get}, storeName =
${storeName})"
}
}
```
(This way we can't define withNewXXX here and have to enforce derived class
to implement them. That's one of cons with this approach.)
And you can derive a new class with last updated snapshot version,
implementing the above trait. You can add more method(s) in the trait if you
can't work with trait and have to depend on the derived class. I'm happy to
help with discussion about extending trait.
I know this proposal would trigger the whole change (I'm sorry about that),
but let's make the code change which is future-proofed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
