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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
