[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1012: IGNITE-17466 Remove TableStorage and PartitionStorage implementations

[GitBox <gi...@apache.org>]

ibessonov commented on code in PR #1012:
URL: https://github.com/apache/ignite-3/pull/1012#discussion_r946665732


##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -393,13 +392,30 @@ public void close() {
         versionChainTree.close();
     }
 
+    /**
+     * Removes all data from this storage and frees all associated resources.
+     *
+     * @throws StorageException If failed to destroy the data or storage is already stopped.
+     */
+    public void destroy() {
+        try {
+            versionChainTree.destroy();

Review Comment:
   Why did you add this? This implementation makes no sense for persistent storage and is just wrong for volatile storage. And, it's synchronous. I don't like it.



##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PartitionlessLinks.java:
##########
@@ -93,8 +93,4 @@ public static void writeToBuffer(ByteBuffer buffer, long link) {
 
         buffer.putInt(pageIndex(link));
     }
-
-    private PartitionlessLinks() {

Review Comment:
   Oh my!



##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/RowVersion.java:
##########
@@ -142,38 +137,45 @@ boolean isCommitted() {
         return timestamp != null;
     }
 
+    /** {@inheritDoc} */
     @Override
     public final void link(long link) {
         this.link = link;
     }
 
+    /** {@inheritDoc} */
     @Override
     public final long link() {
         return link;
     }
 
+    /** {@inheritDoc} */
     @Override
     public final int partition() {
         return partitionId;
     }
 
+    /** {@inheritDoc} */
     @Override
     public int size() {
         assert value != null;
 
         return TIMESTAMP_STORE_SIZE_BYTES + NEXT_LINK_STORE_SIZE_BYTES + VALUE_SIZE_STORE_SIZE_BYTES + value.limit();
     }
 
+    /** {@inheritDoc} */
     @Override
     public int headerSize() {
         return TIMESTAMP_STORE_SIZE_BYTES + NEXT_LINK_STORE_SIZE_BYTES + VALUE_SIZE_STORE_SIZE_BYTES;
     }
 
+    /** {@inheritDoc} */
     @Override
     public IoVersions<? extends AbstractDataPageIo> ioVersions() {

Review Comment:
   Alright, since you modifying this class, may I ask you to change the signature to `public IoVersions<? extends AbstractDataPageIo<RowVersion>> ioVersions()...`? Or even remove a wildcard if it's possible



-- 
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-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org