ibessonov commented on code in PR #3149: URL: https://github.com/apache/ignite-3/pull/3149#discussion_r1477794516
########## modules/core/src/main/java/org/apache/ignite/internal/thread/ThreadAttributes.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.ignite.internal.thread; + +import java.util.Set; + +/** + * Holds some thread attributes. + */ +@SuppressWarnings("InterfaceMayBeAnnotatedFunctional") +public interface ThreadAttributes { + /** + * Returns all operations that this thread allows to execute. + */ + Set<ThreadOperation> allowedOperations(); Review Comment: Would it be convenient to have a method `boolean allowsOperation(ThreadOperation op);`? ########## modules/network/src/main/java/org/apache/ignite/internal/network/netty/NamedNioEventLoopGroup.java: ########## @@ -59,7 +65,9 @@ protected Thread newThread(Runnable r, String unused) { /** * Marker class for network threads. Basically is just a {@link FastThreadLocalThread}. */ - public static class NetworkThread extends FastThreadLocalThread { + public static class NetworkThread extends FastThreadLocalThread implements ThreadAttributes { + private static final Set<ThreadOperation> ALLOWED_OPERATIONS = unmodifiableSet(EnumSet.noneOf(ThreadOperation.class)); Review Comment: Why not simply assign an `emptySet()`? ########## modules/core/src/main/java/org/apache/ignite/internal/util/JavaAssertions.java: ########## @@ -0,0 +1,36 @@ +/* + * 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.util; + +/** + * Code to work with Java language assertions (those that relate to the {@code assert} keyword). + */ +public class JavaAssertions { Review Comment: I think that this is not a good idea. Some developers might abuse methods like this. Generally speaking, all its usages could be replaced with different pattern in code. For example, extracting a method that would contain assertion logic. ########## modules/storage-api/src/main/java/org/apache/ignite/internal/storage/ThreadAssertingMvPartitionStorage.java: ########## @@ -0,0 +1,164 @@ +/* + * 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.storage; + +import static org.apache.ignite.internal.worker.ThreadAssertions.assertThreadAllowsToRead; +import static org.apache.ignite.internal.worker.ThreadAssertions.assertThreadAllowsToWrite; + +import java.util.UUID; +import java.util.concurrent.CompletableFuture; +import org.apache.ignite.internal.hlc.HybridTimestamp; +import org.apache.ignite.internal.schema.BinaryRow; +import org.apache.ignite.internal.storage.gc.GcEntry; +import org.apache.ignite.internal.util.Cursor; +import org.apache.ignite.internal.worker.ThreadAssertions; +import org.jetbrains.annotations.Nullable; + +/** + * {@link MvPartitionStorage} that performs thread assertions when doing read/write operations. + * + * @see ThreadAssertions + */ +public class ThreadAssertingMvPartitionStorage implements MvPartitionStorage { Review Comment: Can you please explain the reason of having a wrapper class instead of putting assertions into `SnapshotAwarePartitionDataStorage`, for example? Or maybe we should put some assertions in places where we use API, not into the implementation. Let me elaborate. For example, what if we have a cursor and somebody reads from it? Should we create wrapper for it as well? Should we create wrapper for all cursors in storages? It makes the code too complicated in my opinion, too many classes and too many wrappers, and what are the benefits? The fact that you would place assertion into a single implementation rather than several implementations for several engines (potentially)? This is arguable. All I see is a code that re-implements a lot of methods of existing interface, forcing us to modify more classes if we modify the interface itself. -- 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]
