[SPARK-35763][SS] Remove the StateStoreCustomMetric subclass enumeration dependency #32914
Conversation
|
cc @HeartSaVioR @xuanyuanking FYI |
There was a problem hiding this comment.
didn't quite get why it worths the effort, you have add code either here or in the case class. no difference to me.
or maybe you can save the effort by provide some default implementation.
There was a problem hiding this comment.
The main point is to avoid making changes here (in SymmetricHashJoinManager) whenever you add a new case class. You just add all the code required in the case class when a new one is added. It is easy to miss adding the code here and with this patch we can avoid exposing the subclasses of StateStoreCustomMetric here.
There was a problem hiding this comment.
+1
this is easier to maintain. exhaustive enumeration of subclasses is brittle to addition of subclasses.
There was a problem hiding this comment.
it would be even better if with some scala magic the default implementation could be provided. but its not obvious to me how, because this is calling the copy constructor which is not available in any trait.
There was a problem hiding this comment.
If we are also considering extension point from outside (like 3rd party state store provider) to add a new implementation of StateStoreCustomMetric, we should definitely not suppose the new implementation to be a case class having desc as field. (Even they have it, we can't leverage it unless doing reflection.) I think this is the right way to do.
|
LGTM once again. thank you for catching the other place where we were doing the same mistake. |
|
Indeed. I was about to comment on the missing spot but it was fixed so fast! Thanks for fixing the overall issue! |
|
add to whitelist |
|
ok to test |
|
Test build #139830 has finished for PR 32914 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Kubernetes integration test starting |
StackOverflowError happened while compiling... I'll retrigger again to see whether it's intermittent or not. |
|
retest this, please |
|
Kubernetes integration test status success |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #139833 has finished for PR 32914 at commit
|
|
Test build #139829 has finished for PR 32914 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #139839 has finished for PR 32914 at commit
|
vkorukanti
changed the title
vkorukanti
changed the title
|
Thanks! Merging to master! |
6 participants
What changes were proposed in this pull request?
Remove the usage of the enumerating subclasses of
StateStoreCustomMetricdependency.To achieve it, add couple of utility methods to
StateStoreCustomMetricwithNewDesc(desc : String)toStateStoreCustomMetricfor cloning the instance with a newdesc(currently used inSymmetricHashJoinStateManager)createSQLMetric(sparkContext: sparkContext): SQLMetricfor creating a correspondingSQLMetricto show the metric in UI and accumulate at the query level (currently used instatefulOperator. stateStoreCustomMetrics)Why are the changes needed?
Code in SymmetricHashJoinStateManager and StateStoreWriter rely on the subclass implementations of StateStoreCustomMetric.
If a new subclass of
StateStoreCustomMetricis added, it requires code changes toSymmetricHashJoinStateManagerandStateStoreWriter, and we may miss the update if there is no existing test coverage.To prevent these issues add a couple of utility methods to
StateStoreCustomMetricas mentioned above.Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing UT and a new UT