Copilot commented on code in PR #7917: URL: https://github.com/apache/ignite-3/pull/7917#discussion_r3028784886
########## modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/inmemory/VolatilePageMemoryDelegate.java: ########## @@ -0,0 +1,114 @@ +/* + * 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.pagememory.inmemory; + +import org.apache.ignite.internal.lang.IgniteInternalCheckedException; +import org.apache.ignite.internal.pagememory.PartitionPageMemory; +import org.apache.ignite.internal.pagememory.io.PageIoRegistry; + +class VolatilePageMemoryDelegate implements PartitionPageMemory { + private final VolatilePageMemory delegate; + private final int groupId; + private final int partitionId; + + VolatilePageMemoryDelegate(VolatilePageMemory delegate, int groupId, int partitionId) { + this.delegate = delegate; + this.groupId = groupId; + this.partitionId = partitionId; + } + + @Override + public int groupId() { + return groupId; + } + + @Override + public int partitionId() { + return partitionId; + } + + @Override + public PageIoRegistry ioRegistry() { + return delegate.ioRegistry(); + } + + @Override + public int pageSize() { + return delegate.pageSize(); + } + + @Override + public int realPageSize(int groupId) { + return delegate.realPageSize(); + } + + @Override + public long allocatePageNoReuse(int groupId, int partitionId, byte flags) throws IgniteInternalCheckedException { + return delegate.allocatePageNoReuse(partitionId, flags); + } Review Comment: `PartitionPageMemory` is described as partition-bound, but the delegate currently accepts arbitrary `groupId/partitionId` parameters and forwards them, allowing accidental cross-partition use. To make the binding meaningful and catch misuse early, add validation (e.g., `assert groupId == this.groupId && partitionId == this.partitionId`) and/or ignore parameters and use the stored `groupId/partitionId` consistently. ########## modules/page-memory/src/testFixtures/java/org/apache/ignite/internal/pagememory/benchmark/PersistentPageMemoryBenchmarkBase.java: ########## @@ -161,6 +169,10 @@ private void createPartitionFilePageStore(int partitionId, int pageSize) throws try { var groupPartitionId = new GroupPartitionId(GROUP_ID, partitionId); + assert !partitionPageMemoryMap.containsKey(groupPartitionId) : partitionPageMemoryMap; + + partitionPageMemoryMap.put(groupPartitionId, persistentPageMemory.createPartitionPageMemory(GROUP_ID, partitionId)); Review Comment: This relies on `assert` to enforce a required invariant, but assertions are often disabled, which would allow overwriting the existing `PartitionPageMemory` silently. Consider using an always-on guard (e.g., `putIfAbsent` + exception on non-null, or an explicit `if` that throws) to avoid hard-to-debug benchmark state corruption. ```suggestion PartitionPageMemory previous = partitionPageMemoryMap.putIfAbsent( groupPartitionId, persistentPageMemory.createPartitionPageMemory(GROUP_ID, partitionId) ); if (previous != null) { throw new IllegalStateException("PartitionPageMemory already exists for " + groupPartitionId + ": " + previous); } ``` ########## modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PartitionPageMemoryDelegate.java: ########## @@ -0,0 +1,116 @@ +/* + * 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.pagememory.persistence; + +import org.apache.ignite.internal.lang.IgniteInternalCheckedException; +import org.apache.ignite.internal.pagememory.PartitionPageMemory; +import org.apache.ignite.internal.pagememory.io.PageIoRegistry; + +class PartitionPageMemoryDelegate implements PartitionPageMemory { + private final PersistentPageMemory delegate; + private final int groupId; + private final int partitionId; + + PartitionPageMemoryDelegate(PersistentPageMemory delegate, int groupId, int partitionId) { + this.delegate = delegate; + this.groupId = groupId; + this.partitionId = partitionId; + } + + @Override + public int groupId() { + return groupId; + } + + @Override + public int partitionId() { + return partitionId; + } + + @Override + public PageIoRegistry ioRegistry() { + return delegate.ioRegistry(); + } + + @Override + public int pageSize() { + return delegate.pageSize(); + } + + @Override + public int realPageSize(int groupId) { + return delegate.realPageSize(groupId); + } + + @Override + public long allocatePageNoReuse(int groupId, int partitionId, byte flags) throws IgniteInternalCheckedException { + return delegate.allocatePageNoReuse(groupId, partitionId, flags); + } + + @Override + public boolean freePage(int groupId, long pageId) { + assert false : "Free page should be never called directly when persistence is enabled."; + + return false; Review Comment: Returning `false` after an `assert false` can silently hide incorrect usage when assertions are disabled in production. Consider throwing an `UnsupportedOperationException` (or a more specific runtime exception) so misuses fail deterministically regardless of JVM assertion settings. ```suggestion throw new UnsupportedOperationException( "Free page should be never called directly when persistence is enabled." ); ``` -- 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]
