dcapwell commented on code in PR #3836:
URL: https://github.com/apache/cassandra/pull/3836#discussion_r1940248636
##########
test/distributed/org/apache/cassandra/distributed/impl/AbstractCluster.java:
##########
@@ -332,29 +333,12 @@ private IInvokableInstance newInstance()
++generation;
IClassTransformer transformer = classTransformer == null ? null :
classTransformer.initialise();
ClassLoader classLoader = new InstanceClassLoader(generation,
config.num(), version.classpath, sharedClassLoader, sharedClassPredicate,
transformer);
- Consumer<Throwable> stabilityInspector;
- {
- try
- {
- Class<?> owner =
classLoader.loadClass(JVMStabilityInspector.class.getName());
- Method method = owner.getMethod("inspectThrowable",
Throwable.class);
Review Comment:
> My problem with the current and prior approach is that we're invoking this
all on another thread via a weird self-referential loop
`Instance.uncaughtException` could prob just call this method, just like we
do for schema
```
@Override
public UUID schemaVersion()
{
// we do not use method reference syntax here, because we need to
sync on the node-local schema instance
//noinspection Convert2MethodRef
return Schema.instance.getVersion();
}
```
Very likely it was done this way because the CL has been hard to work
through and reason, so you just default to always throwing on the executor.
> Passing the Cluster through to each instance to only lookup the instance
itself again is confusing to me, and unnecessary.
I have 0 issues passing the instance to this method. It was done this way
because it was global, now its not; so we can pass the instance through just
fine.
> We can if you like define a static method for this that we mark as
requiring stability for API purposes, or if you really want we can jump through
some extra hoops to invoke the IInstance methods directly without the
indirection via Cluster and additional threads.
My main issue is `UpgradeCluster` and the impact to upgrade testing. As
long as a solution does not regress the behavior and still allows us to test
patch releases and is forward compatible, i am open to options. If we want to
add a new API to jvm-dtest we need to add it to the JVM-Dtest API project and
do a formal release.
--
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: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
