davidnavas commented on a change in pull request #24616: [SPARK-27726] [Core]
Fix performance of ElementTrackingStore deletes when using InMemoryStore under
high loads
URL: https://github.com/apache/spark/pull/24616#discussion_r285792194
##########
File path:
common/kvstore/src/main/java/org/apache/spark/util/kvstore/InMemoryStore.java
##########
@@ -248,16 +337,16 @@ private int compare(T e1, T e2, KVTypeInfo.Accessor
getter) {
diff = compare(e1, natural, natural.get(e2));
}
return diff;
- } catch (Exception e) {
- throw Throwables.propagate(e);
+ } catch (ReflectiveOperationException e) {
+ throw new RuntimeException(e);
}
}
private int compare(T e1, KVTypeInfo.Accessor getter, Object v2) {
try {
return asKey(getter.get(e1)).compareTo(asKey(v2));
Review comment:
I have not attempted to apply the basic trick in getPredicate (of calling or
not calling asKey() depending on whether the index requires it). I agree that
there might be a win there, but the application seems difficult and unlikely to
yield clear code. getPredicate() is doing two things -- it's converting a
Collection into a Set as well as converting all the entries through asKey as
necessary, AND it's removing the necessity of calling getClass.isArray when it
isn't.
There appear three basic times where this strategy would be useful -- during
copyElements when parent is defined, and during iteration when first and/or
last are defined. Well, I don't see a contract about when first or last can be
changed wrt when an iteration is started or running, so I'm a little worried
about modifying that code, and I'm not sure what the usecases are for parents.
But let's assume that first and last are actually fixed during iteration (it
seems bad that it is not), and I'm willing to make changes to the parent code
blind .... Presumably I'd want a set of compare() routines that took a
Comparable as the second argument (the second argument being fixed locally to
asKey(first|last|parent)) with two potential names -- one that would call asKey
and one that wouldn't. I'm blanking on ideas for names. Ideas?
What I'll do is create the "dumb" version of this compare call, and either
we'll revert because we want to allow first and last to be modified during
iteration, or we'll come up with some good names for dealing with the other
half of the asKey calls. Or we'll decide that the getClass.isArray calls for
only one side of the compare isn't so bad.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
