dennishuo commented on code in PR #2527: URL: https://github.com/apache/polaris/pull/2527#discussion_r2381431520
########## persistence/nosql/async/api/src/testFixtures/java/org/apache/polaris/async/AppScopedChecker.java: ########## @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.async; + +import jakarta.enterprise.context.ApplicationScoped; +import java.util.concurrent.atomic.AtomicInteger; + +@ApplicationScoped +class AppScopedChecker { Review Comment: 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? ########## persistence/nosql/async/api/src/main/java/org/apache/polaris/async/AsyncExec.java: ########## @@ -0,0 +1,86 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.async; + +import static java.util.concurrent.Executors.callable; + +import jakarta.enterprise.context.ApplicationScoped; +import java.time.Duration; +import java.util.concurrent.Callable; + +/** + * Abstraction for platform/environment-specific scheduler implementations. + * + * <p>Quarkus production systems use Vert.x, tests usually use Java executors. Review Comment: 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`? ########## persistence/nosql/async/api/src/main/java/org/apache/polaris/async/AsyncConfiguration.java: ########## @@ -0,0 +1,58 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.async; Review Comment: 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? ########## persistence/nosql/async/api/src/main/java/org/apache/polaris/async/Cancelable.java: ########## @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.async; + +import java.util.concurrent.CompletionStage; + +public interface Cancelable<R> { Review Comment: 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? -- 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]
