[GitHub] [ignite-3] ibessonov commented on a diff in pull request #812: IGNITE-16998 [Native Persistence 3.0] Add configuration for checkpoint

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

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


##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/persistence/ItBplusTreePageMemoryImplTest.java:
##########
@@ -55,7 +55,8 @@ protected PageMemory createPageMemory() throws Exception {
                 (page, fullPageId, pageMemoryEx) -> {
                 },
                 (fullPageId, buf, tag) -> {
-                }
+                },
+                PAGE_SIZE

Review Comment:
   PageMemoryImpl has pageSize as a parameter, instead of PageMemoryConfiguration or something. Why?



##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/configuration/schema/PageMemoryDataRegionConfigurationSchema.java:
##########
@@ -57,10 +56,6 @@ public class PageMemoryDataRegionConfigurationSchema {
     @InjectedName
     public String name;
 
-    @Immutable
-    @Value(hasDefault = true)
-    public int pageSize = 16 * 1024;
-
     @Value(hasDefault = true)
     public boolean persistent = false;

Review Comment:
   I believe it would be nice to make this configuration polymorphic. Volatile regions don't need "delay...Write" config, persistent regions don't need minimal size/eviction, and so on



##########
modules/page-memory/src/integrationTest/java/org/apache/ignite/internal/pagememory/tree/ItBplusTreeSelfTest.java:
##########
@@ -2744,7 +2744,8 @@ protected PageMemory createPageMemory() throws Exception {
         return new PageMemoryNoStoreImpl(
                 new UnsafeMemoryProvider(null),
                 dataRegionCfg,
-                ioRegistry
+                ioRegistry,
+                PAGE_SIZE

Review Comment:
   Same here



##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/configuration/schema/PageMemoryStorageEngineConfigurationSchema.java:
##########
@@ -35,6 +40,14 @@ public class PageMemoryStorageEngineConfigurationSchema {
     /** Name of the default data region. */
     public static final String DEFAULT_DATA_REGION_NAME = "default";
 
+    /** Page size in bytes. */
+    @Immutable
+    @Min(1024)
+    @Max(16 * 1024)
+    @PowerOfTwo
+    @Value(hasDefault = true)
+    public int pageSize = 16 * 1024;

Review Comment:
   Ok, what was the point of having new isPow2 for longs if you only check integer values?



##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointWorkflow.java:
##########
@@ -88,28 +83,29 @@ class CheckpointWorkflow implements IgniteComponent {
     private final Collection<PageMemoryDataRegion> dataRegions;
 
     /** Checkpoint write order configuration. */
-    private final CheckpointWriteOrder checkpointWriteOrder;
+    private final String checkpointWriteOrder;

Review Comment:
   You can still use enum, don't you?



-- 
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