NoSQL persistence: add Java/Vert.X executor abstraction layer

[snazy]

Provides an abstraction to submit asynchronous tasks, optionally with a delay or delay + repetition and implementations based on Java's ThreadPoolExecutor and Vert.X.

[RussellSpitzer]

Why do we want an abstraction layer here and why do we need two implementations of that interface? I'm not sure where this fit's into the whole picture of the NoSQL persistence effort.

[snazy]

Why do we want an abstraction layer here and why do we need two implementations of that interface? I'm not sure where this fit's into the whole picture of the NoSQL persistence effort.

There are a couple of task submissions required by the NoSQL work and also the async+reliable tasks work. Within Quarkus it's better to leverage the already present Vert.X timer infrastructure, based on Netty. For testing purposes, there's no Quarkus, so another solution (based on Java's scheduled executor) is needed.

[RussellSpitzer]

I'm not sure I follow, why can't we use Vert.x outside of Quarkus? Doesn't it exist as a stand alone library?

[snazy]

Vert.X is a whole library, not just a thread-pool.

[dimas-b]

Re: Vert.x - it is currently used inside Quarkus. Yet, Quarkus is our standard runtime env. in this project, so I think it is fine for now. If there is a way (or need) to use Vert.x with this executor tool outside of Quarkus, let's do that in a separate follow-up PR.

[RussellSpitzer]

Re: Vert.x - it is currently used inside Quarkus. Yet, Quarkus is our standard runtime env. in this project, so I think it is fine for now. If there is a way (or need) to use Vert.x with this executor tool outside of Quarkus, let's do that in a separate follow-up PR.

My question here is whether we need this PR at all, I'm not sure why we would want an abstraction layer over something which is a required component. I'm not sure having the flexibility here is worth the complexity. This PR is just a building block for something else so I think it's worth considering whether the PR is actually required or whether we can build everything without it. In this case I'm not sure why we wouldn't just have everything using Vert.X and not use an abstraction at all?

[dimas-b]

[...] why we would want an abstraction layer over something which is a required component [...]

PR #1189 uses the java impl. in some test contexts where a full Quarkus server is not available.

We probably could reduce all usage to just the Vert.x implementation, but that would cause (the heavy) Quarkus dependencies to propagate to modules where they would really be excessive.

[RussellSpitzer]

[...] why we would want an abstraction layer over something which is a required component [...]

PR #1189 uses the java impl. in some test contexts where a full Quarkus server is not available.

We probably could reduce all usage to just the Vert.x implementation, but that would cause (the heavy) Quarkus dependencies to propagate to modules where they would really be excessive.

But shouldn't that PR just use the Vert.X implementation in those tests as a library? It doesn't make sense to me that we would add an abstraction on top of a critical library (which effects all of the code written in the project using the library) just so we can avoid using that library during testing. I may not understand the problems here but I feel like there must be a way to run the tests with Vert.X but without Quarkus?

[dimas-b]

[...] there must be a way to run the tests with Vert.X but without Quarkus?

I do not know such a way off the top of my head 😅

... that is without some extra scaffolding, which would then make the scaffolded runtime env. pretty much equivalent to the "plain java" implementation in terms of code correctness validation "power", but the plain impl. is easier, IMHO :)

Edit: running Vert.x in a normal way (e.g. in unit tests), still pulls in the whole framework, including Netty, so it is a lot heavier than the java impl.

[flyrain]

Looks like this is only used within MangoDB persistence. I'd suggest a different package name.

[dimas-b]

Package names are meant to represent what the code is the package "is" or "does", not where it is used 🙂 The current package rename reflects the purpose of the contained code well, IMHO.

[flyrain]

I see your point, but I think usage context matters too. If a package name only describes what the code “is/does,” it could become too generic. Tying the name to where and how it’s used helps maintain clarity and avoids misplacement over time.

[dimas-b]

Suppose we had the package name org.apache.polaris.nosql.async and one week from now reuse it for handling events? Would that package name work in the events context?

The code is already under the nosql directory name (as requested in other discussions). That provides isolation for current use cases. However, if it later becomes used in another context, it will be possible to relocate the module to another directory (for visibility / sharing) without affecting existing code with package renames.

[dennishuo]

That's an interesting consideration. Generally I'd agree that abstractly useful packages shouldn't bake assumed use sites into its package name. On the other hand, the reusability of packages should be a somewhat intentional choice, and a lot of the injection/application-scoped patterns in these impls make it hard to reason about what it should be used for or what it's safe to be used for.

For example, naively just diving in without context, as a developer trying to assess what this package does and if it would work for some new feature I'm adding elsewhere in Polaris, I see JavaPoolAsyncExec being annotated for injection as ApplicationScoped and then there's

private static final AtomicInteger POOL_ID = new AtomicInteger();
private final int poolId = POOL_ID.incrementAndGet();

It's unclear whether I'm supposed to use some process-wide shared pool or spin up a new pool for the task type, etc.

Also, as the scope implies broad usage for any "async" things, we'd want to more clearly document the semantics of the various wrapper idioms and how they relate to either Vertx or to Java core concurrency libraries. For example, naively jumping in it's hard to understand how to wield the interface, and why there's a Cancelable interface defined here in Polaris and how that relates to cancelling a Future. My guess would've been if it's trying to be a common interface between two async exec frameworks that don't consistently use Futures, but then Cancelable has a completionStage method and CompletionStage has toCompletableFuture(), so it's unclear what that means. And both actual AsyncExec impls in here actually to just return their instances of CancelableFuture which is a private final class CancelableFuture<R> implements Cancelable<R>, Runnable defined in AsyncExec.

So the caller will get something called CancelableFuture but the public interface only gives you a Cancelable, and a CancelableFuture doesn't even implement Future, but apparently it contains a Future which has a cancel method. And this AsyncExec interface is basically like ScheduledExecutorService, except it returns these Cancelable instead of ScheduledFuture.

If we want to make this a more broadly used library within Polaris we can do it but it'll just need a lot more scrutiny, compared to chalking it up to idiosyncracies of usage in a particular submodule.

For me, maybe it's just that I'm not as familiar with Vertx. Is this Cancelable stuff a Vertx idiom?

[dimas-b]

You bring good points / questions, @dennishuo . I think it's certainly possible to get a more thorough discussion about them... However, I wonder if it might be preferable to do that on the dev ML.

From the practical point of view the Polaris codebase is still in rapid development are we're adding a lot of code in other areas too. Polishing everything to perfection is probably going to slow down progress to crawl 😅

So, to allow the bigger NoSQL feature work to progress, would you be ok with renaming the package to something NoSQL-specific for now so that prospective users are less likely to assume it is "trivial" to use in other contexts?

My suggestion: org.apache.polaris.nodes.async (since it is currently used only within the node management code in #1189).

I think adding some more javadoc in this PR would be reasonable, but I'm not sure adding exhaustive javadoc would be practical.

@dennishuo @flyrain WDYT?

[dimas-b]

The package name is org.apache.polaris.nosql.async now... Does it work from you POV?

[dennishuo]

Narrowing the package name to org.apache.polaris.nosql.async for now works for me. If we ever want to open it up to broader use we should be ready to discuss and change the Cancelable and AsyncExec interfaces.

[dimas-b]

Sounds good! Thanks for the review @dennishuo !

[dimas-b]

Re: Abstraction: The "api" module defines the functionality expected by other modules for executing tasks inside the JVM. It is not quite the same as the usual java "executor" / "scheduler" interfaces, but defines something more specific and smaller in terms of API surface.

The Vert.x implementation is provided to take advantage of the runtime efficiencies within Quarkus.

The "java" implementation is provided for use in non-Quarkus environments (e.g. tests). It is lighter than Vert.x (in runtime). Another point for having the java impl. is to validate the Vert.x impl. by having and alternative implementation (kind of like solving a math problem in two ways for extra certainty). The java impl. can also be used as a fallback, should unexpected issues come up with the Vert.x impl. (during dependency upgrades, for example).

[dennishuo]

That's an interesting consideration. Generally I'd agree that abstractly useful packages shouldn't bake assumed use sites into its package name. On the other hand, the reusability of packages should be a somewhat intentional choice, and a lot of the injection/application-scoped patterns in these impls make it hard to reason about what it should be used for or what it's safe to be used for.

For example, naively just diving in without context, as a developer trying to assess what this package does and if it would work for some new feature I'm adding elsewhere in Polaris, I see JavaPoolAsyncExec being annotated for injection as ApplicationScoped and then there's

private static final AtomicInteger POOL_ID = new AtomicInteger();
private final int poolId = POOL_ID.incrementAndGet();

It's unclear whether I'm supposed to use some process-wide shared pool or spin up a new pool for the task type, etc.

Also, as the scope implies broad usage for any "async" things, we'd want to more clearly document the semantics of the various wrapper idioms and how they relate to either Vertx or to Java core concurrency libraries. For example, naively jumping in it's hard to understand how to wield the interface, and why there's a Cancelable interface defined here in Polaris and how that relates to cancelling a Future. My guess would've been if it's trying to be a common interface between two async exec frameworks that don't consistently use Futures, but then Cancelable has a completionStage method and CompletionStage has toCompletableFuture(), so it's unclear what that means. And both actual AsyncExec impls in here actually to just return their instances of CancelableFuture which is a private final class CancelableFuture<R> implements Cancelable<R>, Runnable defined in AsyncExec.

So the caller will get something called CancelableFuture but the public interface only gives you a Cancelable, and a CancelableFuture doesn't even implement Future, but apparently it contains a Future which has a cancel method. And this AsyncExec interface is basically like ScheduledExecutorService, except it returns these Cancelable instead of ScheduledFuture.

If we want to make this a more broadly used library within Polaris we can do it but it'll just need a lot more scrutiny, compared to chalking it up to idiosyncracies of usage in a particular submodule.

For me, maybe it's just that I'm not as familiar with Vertx. Is this Cancelable stuff a Vertx idiom?

[dennishuo]

Even if not using Java executors, can't we still use Java interfaces like ScheduledExecutorService? Is Vertx fundamentally incompatible with ScheduledFutures that are returned in ScheduledExecutorService?

[dennishuo]

Can we have some javadoc comments explaining the difference between this and Future and why we want to partially hide the Futures returned by executors under this interface?

Can callers still get a future via completionStage().toCompletableFuture()? Why is it a "has a future" pattern instead of an "is a future" pattern?

[dennishuo]

Could use a javadoc for how this is supposed to be used in tests. Does it make sense to use this across different usage domains? Like if we had some persistence and some events-handling features all using this package, and an integration test exercises both, is there a semantic to sharing a global singleton AtomicInteger between them via injection?

Is the CDI usage important here compared to just declaring a static final AtomicInteger within the test class itself?

[dennishuo]

With the move to the more contained package, and the expanded javadoc comments, I'm okay going ahead with this. If we want to start using this outside of the nosql-specific code I'd want to dig a bit more into the necessity of the Cancelable and AsyncExec interfaces, in contrast to structuring any wrappers as adapters over core Java interfaces instead.

[snazy]

Thanks all, merging.