korlov42 commented on code in PR #4799: URL: https://github.com/apache/ignite-3/pull/4799#discussion_r1865394746
########## modules/sql-engine-api/src/main/java/org/apache/ignite/internal/sql/common/cancel/api/CancelHandlerRegistry.java: ########## @@ -0,0 +1,41 @@ +/* + * 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.ignite.internal.sql.common.cancel.api; Review Comment: proper name is `org.apache.ignite.internal.sql.engine.api.cancel` (public|internal package prefix must be followed by name of the module ) ########## modules/sql-engine-api/src/main/java/org/apache/ignite/internal/sql/common/cancel/api/CancelHandlerRegistry.java: ########## @@ -0,0 +1,41 @@ +/* + * 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.ignite.internal.sql.common.cancel.api; + +/** + * Registry of handlers that can cancel a specific operation. + */ +public interface CancelHandlerRegistry { + /** + * Registers a cancel handler. + * + * @param type Type of the cancellable operation. + * @param handler Handler to register. + * @param local {@code True} if the handler can cancel operations only on local node, + * {@code false} if the handler can cancel operations across the entire cluster. + */ + void register(CancellableOperationType type, OperationCancelHandler handler, boolean local); Review Comment: I assume you've introduced 3 params to simplify integration (just pass lambda cancelling an object), but all these attributes (type and locality) in fact belong to `OperationCancelHandler`: you cannot use the same lambda for local and non-local cancellation (well, it will work if handler provide cluster-wide cancellation, but in general it doesn't work), the same with operation type. I propose to make these attributes part of `OperationCancelHandler` interface. ########## modules/sql-engine-api/src/main/java/org/apache/ignite/internal/sql/common/cancel/api/CancelHandlerRegistry.java: ########## @@ -0,0 +1,41 @@ +/* + * 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.ignite.internal.sql.common.cancel.api; + +/** + * Registry of handlers that can cancel a specific operation. + */ +public interface CancelHandlerRegistry { Review Comment: name `CancelHandlerRegistry` clashes with `CancelHandle` from public API which is, although of the same area, still a bit different entity. I would name it `KillHandlerRegistry` to make it clear this registry relates to sql's KILL statements processing. ########## modules/sql-engine-api/src/main/java/org/apache/ignite/internal/sql/common/cancel/api/CancelHandlerRegistry.java: ########## @@ -0,0 +1,41 @@ +/* + * 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.ignite.internal.sql.common.cancel.api; + +/** + * Registry of handlers that can cancel a specific operation. + */ +public interface CancelHandlerRegistry { + /** + * Registers a cancel handler. + * + * @param type Type of the cancellable operation. + * @param handler Handler to register. + * @param local {@code True} if the handler can cancel operations only on local node, + * {@code false} if the handler can cancel operations across the entire cluster. + */ + void register(CancellableOperationType type, OperationCancelHandler handler, boolean local); + + /** + * Returns a handler that can cancel an operation of the specified type across the entire cluster. + * + * @param type Type of the cancellable operation. + * @return Handler that can cancel an operation of the specified type across the entire cluster. + */ + OperationCancelHandler handler(CancellableOperationType type); Review Comment: why do we need getter in API? ########## modules/sql-engine-api/src/main/java/org/apache/ignite/internal/sql/common/cancel/api/CancelHandlerRegistry.java: ########## @@ -0,0 +1,41 @@ +/* + * 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.ignite.internal.sql.common.cancel.api; + +/** + * Registry of handlers that can cancel a specific operation. + */ +public interface CancelHandlerRegistry { + /** + * Registers a cancel handler. + * + * @param type Type of the cancellable operation. + * @param handler Handler to register. + * @param local {@code True} if the handler can cancel operations only on local node, + * {@code false} if the handler can cancel operations across the entire cluster. + */ + void register(CancellableOperationType type, OperationCancelHandler handler, boolean local); Review Comment: I've just discovered `CancelHandlerDescriptor`, and this proves my assumption above :) ########## modules/sql-engine-api/src/main/java/org/apache/ignite/internal/sql/common/cancel/api/OperationCancelHandler.java: ########## @@ -0,0 +1,41 @@ +/* + * 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.ignite.internal.sql.common.cancel.api; + +import java.util.UUID; +import java.util.concurrent.CompletableFuture; + +/** + * Handler that can cancel operations of a certain type. + * + * <p>When registering a handler in the {@link CancelHandlerRegistry registry}, you must specify + * whether the handler can cancel the operation on the local node only or on the entire cluster. + * + * @see CancelHandlerRegistry#register(CancellableOperationType, OperationCancelHandler, boolean) + */ +@FunctionalInterface +public interface OperationCancelHandler { + /** + * Cancels an operation with the specified ID. + * + * @param objectId ID of the operation to cancel. + * @return {@code true} if the operation was successfully canceled, + * {@code false} if a specific operation was not found or is inactive. + */ + CompletableFuture<Boolean> cancelAsync(UUID objectId); Review Comment: Does it make sense to make `cancelAsync` accept string instead of UUID? By requesting UUID, we force others to identify their cancellable by UUID, while sql engine doesn't actually care about particular type. More over, system views already returns VARCHAR for id columns, the same with KILL statements -- id parameter is of type of string. -- 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: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
