ivanzlenko commented on code in PR #7969:
URL: https://github.com/apache/ignite-3/pull/7969#discussion_r3071069741
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/logit/LogitLogStorageManager.java:
##########
@@ -101,19 +101,19 @@ public CompletableFuture<Void> stopAsync(ComponentContext
componentContext) {
}
@Override
- public LogStorage createLogStorage(String groupId, RaftOptions
raftOptions) {
- Requires.requireTrue(StringUtils.isNotBlank(groupId), "Blank log
storage uri.");
+ public LogStorage createLogStorage(String raftNodeStorageId, RaftOptions
raftOptions) {
+ Requires.requireTrue(StringUtils.isNotBlank(raftNodeStorageId), "Blank
log storage uri.");
Review Comment:
Is it raftNodeStorageId or log storage uri? Let's rename it in different PR.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentLogStorageManager.java:
##########
@@ -108,21 +111,25 @@ public CompletableFuture<Void> stopAsync(ComponentContext
componentContext) {
}
}
- private static long convertGroupId(String groupId) {
- if ("metastorage_group".equals(groupId)) {
- return 1;
+ private static long convertNodeId(String nodeId) {
+ // Temporary approach, will be revised after changing partition ID
from int to long.
+
+ StoredRaftNodeId raftNodeId =
RaftNodeId.fromNodeIdStringForStorage(nodeId, "");
+ if ("metastorage_group".equals(raftNodeId.groupIdName())) {
+ return 1 + raftNodeId.peer().idx();
}
- if ("cmg_group".equals(groupId)) {
- return 2;
+ if (nodeId.contains("cmg_group")) {
+ return raftNodeId.peer().idx() + 1000;
}
- String[] partitionGroupIdArray =
PARTITION_GROUP_ID_PATTERN.split(groupId);
+ String[] partitionGroupIdArray =
PARTITION_GROUP_ID_PATTERN.split(raftNodeId.groupIdName());
if (partitionGroupIdArray.length == 2) {
- return Long.parseLong(partitionGroupIdArray[0]) << 32 |
Long.parseLong(partitionGroupIdArray[1]);
+ return (parseLong(partitionGroupIdArray[0]) << 32 |
parseLong(partitionGroupIdArray[1])) + 100000 + raftNodeId.peer().idx();
Review Comment:
Let's move it into a separate method. Calculation getting too complex. Also
worth explaining what is going on here in javaDoc.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/util/SharedLogStorageManagerUtils.java:
##########
@@ -41,36 +49,67 @@ public class SharedLogStorageManagerUtils {
* LOGIT_STORAGE_ENABLED_PROPERTY and fsync set to true.
*/
@TestOnly
- public static LogStorageManager create(String nodeName, Path
logStoragePath) {
- return create("test", nodeName, logStoragePath, true);
+ public static LogStorageManager create(String nodeName, Path
logStoragePath, LogStorageConfiguration logStorageConfig) {
+ return create("test", nodeName, logStoragePath, true,
logStorageConfig);
}
/**
* Creates a LogStorageManager with {@link DefaultLogStorageManager} or
{@link LogitLogStorageManager} implementation depending on
* LOGIT_STORAGE_ENABLED_PROPERTY.
*/
+ @TestOnly
public static LogStorageManager create(
String factoryName,
String nodeName,
Path logStoragePath,
- boolean fsync
+ boolean fsync,
+ LogStorageConfiguration logStorageConfig
) {
- return create(factoryName, nodeName, logStoragePath, fsync,
RocksDbLogStorageOptions.defaults());
+ return create(
+ factoryName,
+ nodeName,
+ logStoragePath,
+ fsync,
+ RocksDbLogStorageOptions.defaults(),
+ new SegmentLogStorageOptions(1, logStorageConfig, new
FailureManager(new NoOpFailureHandler()))
+ );
}
/**
- * Creates a LogStorageManager with {@link DefaultLogStorageManager} or
{@link LogitLogStorageManager} implementation depending on
- * LOGIT_STORAGE_ENABLED_PROPERTY.
+ * Creates a LogStorageManager with {@link SegmentLogStorageManager},
{@link DefaultLogStorageManager} or {@link LogitLogStorageManager}
+ * implementation depending on SEGSTORE_ENABLED_PROPERTY /
LOGIT_STORAGE_ENABLED_PROPERTY.
*/
public static LogStorageManager create(
String factoryName,
String nodeName,
Path logStoragePath,
boolean fsync,
- RocksDbLogStorageOptions specificOptions
+ RocksDbLogStorageOptions specificOptions,
Review Comment:
parameter worth renaming
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentLogStorageManager.java:
##########
@@ -108,21 +111,25 @@ public CompletableFuture<Void> stopAsync(ComponentContext
componentContext) {
}
}
- private static long convertGroupId(String groupId) {
- if ("metastorage_group".equals(groupId)) {
- return 1;
+ private static long convertNodeId(String nodeId) {
+ // Temporary approach, will be revised after changing partition ID
from int to long.
Review Comment:
Please add a TODO if it is actually temporary. Otherwise it will become
permanent.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentLogStorageManager.java:
##########
@@ -108,21 +111,25 @@ public CompletableFuture<Void> stopAsync(ComponentContext
componentContext) {
}
}
- private static long convertGroupId(String groupId) {
- if ("metastorage_group".equals(groupId)) {
- return 1;
+ private static long convertNodeId(String nodeId) {
+ // Temporary approach, will be revised after changing partition ID
from int to long.
+
+ StoredRaftNodeId raftNodeId =
RaftNodeId.fromNodeIdStringForStorage(nodeId, "");
+ if ("metastorage_group".equals(raftNodeId.groupIdName())) {
+ return 1 + raftNodeId.peer().idx();
}
- if ("cmg_group".equals(groupId)) {
- return 2;
+ if (nodeId.contains("cmg_group")) {
+ return raftNodeId.peer().idx() + 1000;
Review Comment:
Please avoid magic numbers - should be documented constant.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/util/SharedLogStorageManagerUtils.java:
##########
@@ -41,36 +49,67 @@ public class SharedLogStorageManagerUtils {
* LOGIT_STORAGE_ENABLED_PROPERTY and fsync set to true.
*/
@TestOnly
- public static LogStorageManager create(String nodeName, Path
logStoragePath) {
- return create("test", nodeName, logStoragePath, true);
+ public static LogStorageManager create(String nodeName, Path
logStoragePath, LogStorageConfiguration logStorageConfig) {
+ return create("test", nodeName, logStoragePath, true,
logStorageConfig);
}
/**
* Creates a LogStorageManager with {@link DefaultLogStorageManager} or
{@link LogitLogStorageManager} implementation depending on
* LOGIT_STORAGE_ENABLED_PROPERTY.
*/
+ @TestOnly
Review Comment:
We are using this method in just 3 places. Is it worth to expose another
TestOnly method which is bad by itself?
I would've just use the new method and that's it.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentLogStorageManager.java:
##########
@@ -108,21 +111,25 @@ public CompletableFuture<Void> stopAsync(ComponentContext
componentContext) {
}
}
- private static long convertGroupId(String groupId) {
- if ("metastorage_group".equals(groupId)) {
- return 1;
+ private static long convertNodeId(String nodeId) {
+ // Temporary approach, will be revised after changing partition ID
from int to long.
+
+ StoredRaftNodeId raftNodeId =
RaftNodeId.fromNodeIdStringForStorage(nodeId, "");
+ if ("metastorage_group".equals(raftNodeId.groupIdName())) {
+ return 1 + raftNodeId.peer().idx();
}
- if ("cmg_group".equals(groupId)) {
- return 2;
+ if (nodeId.contains("cmg_group")) {
+ return raftNodeId.peer().idx() + 1000;
}
- String[] partitionGroupIdArray =
PARTITION_GROUP_ID_PATTERN.split(groupId);
+ String[] partitionGroupIdArray =
PARTITION_GROUP_ID_PATTERN.split(raftNodeId.groupIdName());
if (partitionGroupIdArray.length == 2) {
- return Long.parseLong(partitionGroupIdArray[0]) << 32 |
Long.parseLong(partitionGroupIdArray[1]);
+ return (parseLong(partitionGroupIdArray[0]) << 32 |
parseLong(partitionGroupIdArray[1])) + 100000 + raftNodeId.peer().idx();
} else {
- throw new IllegalArgumentException("Invalid groupId: " + groupId);
+ // For tests using invalid group IDs.
Review Comment:
That's not a good approach. Let's revert to IllegalArgumentException.
Otherwise we can just miss very nasty bugs.
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -664,19 +666,30 @@ public class IgniteImpl implements Ignite {
RaftConfiguration raftConfiguration =
nodeConfigRegistry.getConfiguration(RaftExtensionConfiguration.KEY).raft();
+ LogStorageExtensionConfiguration logStorageConfig =
nodeConfigRegistry.getConfiguration(LogStorageExtensionConfiguration.KEY);
+
+ SegmentLogStorageOptions segstoreSpecificOptions = null;
+ if (logStorageConfig != null) {
Review Comment:
It's a bad approach. Why not just create partitionsLogStorageManager here?
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentLogStorageManager.java:
##########
@@ -108,21 +111,25 @@ public CompletableFuture<Void> stopAsync(ComponentContext
componentContext) {
}
}
- private static long convertGroupId(String groupId) {
- if ("metastorage_group".equals(groupId)) {
- return 1;
+ private static long convertNodeId(String nodeId) {
Review Comment:
Is it covered by any tests?
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/util/SharedLogStorageManagerUtils.java:
##########
@@ -41,36 +49,67 @@ public class SharedLogStorageManagerUtils {
* LOGIT_STORAGE_ENABLED_PROPERTY and fsync set to true.
*/
@TestOnly
- public static LogStorageManager create(String nodeName, Path
logStoragePath) {
- return create("test", nodeName, logStoragePath, true);
+ public static LogStorageManager create(String nodeName, Path
logStoragePath, LogStorageConfiguration logStorageConfig) {
+ return create("test", nodeName, logStoragePath, true,
logStorageConfig);
}
/**
* Creates a LogStorageManager with {@link DefaultLogStorageManager} or
{@link LogitLogStorageManager} implementation depending on
* LOGIT_STORAGE_ENABLED_PROPERTY.
*/
+ @TestOnly
public static LogStorageManager create(
String factoryName,
String nodeName,
Path logStoragePath,
- boolean fsync
+ boolean fsync,
+ LogStorageConfiguration logStorageConfig
) {
- return create(factoryName, nodeName, logStoragePath, fsync,
RocksDbLogStorageOptions.defaults());
+ return create(
+ factoryName,
+ nodeName,
+ logStoragePath,
+ fsync,
+ RocksDbLogStorageOptions.defaults(),
+ new SegmentLogStorageOptions(1, logStorageConfig, new
FailureManager(new NoOpFailureHandler()))
+ );
}
/**
- * Creates a LogStorageManager with {@link DefaultLogStorageManager} or
{@link LogitLogStorageManager} implementation depending on
- * LOGIT_STORAGE_ENABLED_PROPERTY.
+ * Creates a LogStorageManager with {@link SegmentLogStorageManager},
{@link DefaultLogStorageManager} or {@link LogitLogStorageManager}
+ * implementation depending on SEGSTORE_ENABLED_PROPERTY /
LOGIT_STORAGE_ENABLED_PROPERTY.
*/
public static LogStorageManager create(
String factoryName,
String nodeName,
Path logStoragePath,
boolean fsync,
- RocksDbLogStorageOptions specificOptions
+ RocksDbLogStorageOptions specificOptions,
+ SegmentLogStorageOptions segmentStoreSpecificOptions
Review Comment:
So, effectively if we will make a separate method for
segmentStoreSpecificOptions you will not need a logStorageConfig and you will
not need all test changes you have made.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentLogStorageManager.java:
##########
@@ -108,21 +111,25 @@ public CompletableFuture<Void> stopAsync(ComponentContext
componentContext) {
}
}
- private static long convertGroupId(String groupId) {
- if ("metastorage_group".equals(groupId)) {
- return 1;
+ private static long convertNodeId(String nodeId) {
+ // Temporary approach, will be revised after changing partition ID
from int to long.
+
+ StoredRaftNodeId raftNodeId =
RaftNodeId.fromNodeIdStringForStorage(nodeId, "");
+ if ("metastorage_group".equals(raftNodeId.groupIdName())) {
+ return 1 + raftNodeId.peer().idx();
}
- if ("cmg_group".equals(groupId)) {
- return 2;
+ if (nodeId.contains("cmg_group")) {
+ return raftNodeId.peer().idx() + 1000;
}
- String[] partitionGroupIdArray =
PARTITION_GROUP_ID_PATTERN.split(groupId);
+ String[] partitionGroupIdArray =
PARTITION_GROUP_ID_PATTERN.split(raftNodeId.groupIdName());
if (partitionGroupIdArray.length == 2) {
- return Long.parseLong(partitionGroupIdArray[0]) << 32 |
Long.parseLong(partitionGroupIdArray[1]);
+ return (parseLong(partitionGroupIdArray[0]) << 32 |
parseLong(partitionGroupIdArray[1])) + 100000 + raftNodeId.peer().idx();
Review Comment:
At least let's add an assertion
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/util/SharedLogStorageManagerUtils.java:
##########
@@ -41,36 +49,67 @@ public class SharedLogStorageManagerUtils {
* LOGIT_STORAGE_ENABLED_PROPERTY and fsync set to true.
*/
@TestOnly
- public static LogStorageManager create(String nodeName, Path
logStoragePath) {
- return create("test", nodeName, logStoragePath, true);
+ public static LogStorageManager create(String nodeName, Path
logStoragePath, LogStorageConfiguration logStorageConfig) {
+ return create("test", nodeName, logStoragePath, true,
logStorageConfig);
}
/**
* Creates a LogStorageManager with {@link DefaultLogStorageManager} or
{@link LogitLogStorageManager} implementation depending on
* LOGIT_STORAGE_ENABLED_PROPERTY.
*/
+ @TestOnly
public static LogStorageManager create(
String factoryName,
String nodeName,
Path logStoragePath,
- boolean fsync
+ boolean fsync,
+ LogStorageConfiguration logStorageConfig
) {
- return create(factoryName, nodeName, logStoragePath, fsync,
RocksDbLogStorageOptions.defaults());
+ return create(
+ factoryName,
+ nodeName,
+ logStoragePath,
+ fsync,
+ RocksDbLogStorageOptions.defaults(),
+ new SegmentLogStorageOptions(1, logStorageConfig, new
FailureManager(new NoOpFailureHandler()))
+ );
}
/**
- * Creates a LogStorageManager with {@link DefaultLogStorageManager} or
{@link LogitLogStorageManager} implementation depending on
- * LOGIT_STORAGE_ENABLED_PROPERTY.
+ * Creates a LogStorageManager with {@link SegmentLogStorageManager},
{@link DefaultLogStorageManager} or {@link LogitLogStorageManager}
+ * implementation depending on SEGSTORE_ENABLED_PROPERTY /
LOGIT_STORAGE_ENABLED_PROPERTY.
*/
public static LogStorageManager create(
String factoryName,
String nodeName,
Path logStoragePath,
boolean fsync,
- RocksDbLogStorageOptions specificOptions
+ RocksDbLogStorageOptions specificOptions,
+ SegmentLogStorageOptions segmentStoreSpecificOptions
Review Comment:
You are missing nullable annotation here. But it is better just to create a
separate method with proper name and call it where appropriate?
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/storage/segstore/SegmentLogStorageManager.java:
##########
@@ -108,21 +111,25 @@ public CompletableFuture<Void> stopAsync(ComponentContext
componentContext) {
}
}
- private static long convertGroupId(String groupId) {
- if ("metastorage_group".equals(groupId)) {
- return 1;
+ private static long convertNodeId(String nodeId) {
+ // Temporary approach, will be revised after changing partition ID
from int to long.
+
+ StoredRaftNodeId raftNodeId =
RaftNodeId.fromNodeIdStringForStorage(nodeId, "");
+ if ("metastorage_group".equals(raftNodeId.groupIdName())) {
+ return 1 + raftNodeId.peer().idx();
}
- if ("cmg_group".equals(groupId)) {
- return 2;
+ if (nodeId.contains("cmg_group")) {
+ return raftNodeId.peer().idx() + 1000;
}
- String[] partitionGroupIdArray =
PARTITION_GROUP_ID_PATTERN.split(groupId);
+ String[] partitionGroupIdArray =
PARTITION_GROUP_ID_PATTERN.split(raftNodeId.groupIdName());
if (partitionGroupIdArray.length == 2) {
Review Comment:
Do we have any documentation or limitation checks to validate that we can't
get past these magic numbers? Otherwise we could end up in a world with
possible collisions betwen raft group ids.
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -664,19 +666,30 @@ public class IgniteImpl implements Ignite {
RaftConfiguration raftConfiguration =
nodeConfigRegistry.getConfiguration(RaftExtensionConfiguration.KEY).raft();
+ LogStorageExtensionConfiguration logStorageConfig =
nodeConfigRegistry.getConfiguration(LogStorageExtensionConfiguration.KEY);
+
+ SegmentLogStorageOptions segstoreSpecificOptions = null;
+ if (logStorageConfig != null) {
+ segstoreSpecificOptions = new SegmentLogStorageOptions(
+ raftConfiguration.disruptor().logManagerStripes().value(),
+ logStorageConfig.logStorage(),
+ failureManager
+ );
+ }
+
// TODO https://issues.apache.org/jira/browse/IGNITE-19051
RaftGroupEventsClientListener raftGroupEventsClientListener = new
RaftGroupEventsClientListener();
partitionsWorkDir = partitionsPath(systemConfiguration, workDir);
InternalClusterNode localNode = clusterSvc.staticLocalNode();
Review Comment:
Why this blank line got removed?
--
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]
