sanpwc commented on a change in pull request #642:
URL: https://github.com/apache/ignite-3/pull/642#discussion_r829055639
##########
File path:
modules/metastorage-client/src/main/java/org/apache/ignite/internal/metastorage/client/MetaStorageService.java
##########
@@ -31,6 +31,20 @@
* Defines interface for access to a meta storage service.
*/
public interface MetaStorageService {
+ /**
+ * Retrieves a current revision.
+ *
+ * @return Revision.
+ */
+ CompletableFuture<Long> revision();
Review comment:
Both introduced methods should be removed, ya?
##########
File path:
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java
##########
@@ -98,6 +103,12 @@
*/
private static final Path PARTITIONS_STORE_PATH = Paths.get("db");
+ /**
+ * Difference between the local node applied revision and distributed data
storage revision on start.
+ * TODO: IGNITE-16488 Move the property to configuration.
+ */
+ public static final int METADATA_DIFFERENCE = 100;
Review comment:
private
##########
File path:
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java
##########
@@ -192,219 +194,127 @@ public TableManager(
engine = new RocksDbStorageEngine();
- tablesVv = new VersionedValue<>(registry);
- tablesByIdVv = new VersionedValue<>(registry);
+ tablesVv = new VersionedValue<>(registry, HashMap::new);
+ tablesByIdVv = new VersionedValue<>(registry, HashMap::new);
}
/** {@inheritDoc} */
@Override
public void start() {
- tablesCfg.tables()
- .listenElements(new ConfigurationNamedListListener<>() {
- @Override
- public CompletableFuture<?>
onCreate(ConfigurationNotificationEvent<TableView> ctx) {
- if (!busyLock.enterBusy()) {
- String tblName = ctx.newValue().name();
- UUID tblId = ((ExtendedTableView)
ctx.newValue()).id();
-
- fireEvent(TableEvent.CREATE,
- new
TableEventParameters(ctx.storageRevision(), tblId, tblName),
- new NodeStoppingException()
- );
-
- return CompletableFuture.failedFuture(new
NodeStoppingException());
- }
+ ((ExtendedTableConfiguration)
tablesCfg.tables().any()).schemas().listenElements(new
ConfigurationNamedListListener<>() {
Review comment:
In order to improve the readability I'd rather extract all
listener-reaction-logic to explicit methods
```
((ExtendedTableConfiguration)
tablesCfg.tables().any()).schemas().listenElements(new
ConfigurationNamedListListener<>() {
@Override
public CompletableFuture<?>
onCreate(ConfigurationNotificationEvent<SchemaView> schemasCtx) {
return **onSchemaCreate();**
}
}
((ExtendedTableConfiguration)
tablesCfg.tables().any()).assignments().listen(() -> **onAssignmentsChange()**);
...
```
##########
File path:
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java
##########
@@ -164,7 +167,121 @@ public void nodeWithDataTest(TestInfo testInfo) {
+ " }\n"
+ "}", workDir);
- TableDefinition scmTbl1 = SchemaBuilders.tableBuilder("PUBLIC",
TABLE_NAME).columns(
+ createTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+
+ IgnitionManager.stop(nodeName);
+
+ ignite = IgnitionManager.start(nodeName, null, workDir);
+
+ checkTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+
+ IgnitionManager.stop(nodeName);
+ }
+
+ /**
+ * Starts two nodes and checks that the data are storing through restarts.
+ * Nodes restart in the same order when they started at first.
+ *
+ * @param testInfo Test information object.
+ */
+ @Test
+ public void testTwoNodesRestartDirect(TestInfo testInfo) {
+ twoNodesRestart(testInfo, true);
+ }
+
+ /**
+ * Starts two nodes and checks that the data are storing through restarts.
+ * Nodes restart in reverse order when they started at first.
+ *
+ * @param testInfo Test information object.
+ */
+ @Test
+ @Disabled("IGNITE-16034 Unblock a node start that happenes before
Metastorage is ready")
+ public void testTwoNodesRestartReverse(TestInfo testInfo) {
+ twoNodesRestart(testInfo, false);
+ }
+
+ /**
+ * Starts two nodes and checks that the data are storing through restarts.
+ *
+ * @param testInfo Test information object.
+ * @param directOrder When the parameter is true, nodes restart in direct
order, otherwise they restart in reverse order.
+ */
+ private void twoNodesRestart(TestInfo testInfo, boolean directOrder) {
Review comment:
We definitely need to check scenarios where
1. Node that was stopped received and applied all meta when it was alive.
2. Skipped some meta that will be recovered during node recovery process.
Seems that given two-node-tests aren't enough specific, we either don't
assert that all nodes apply all meta nor check second option.
Besides that we should add recovery tests that check whether data itself
(not meta data) is available on restored node. In other words we should add
tests for replica factor 3 + leader change or direct storage checks.
##########
File path:
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java
##########
@@ -363,6 +382,98 @@ public void start(@Nullable String cfg) {
}
}
+ /**
+ * Whether the node had started in standalone mode (for example,
maintenance mode).
+ *
+ * @return Whether the node had started in standalone mode.
+ */
+ private boolean standaloneMode() {
+ return false;
+ }
+
+ /**
+ * Awaits for a permission to join the cluster, i.e. node join response
from Cluster Management group.
+ * After the completion of this method, the node is considered as
validated.
+ */
+ private void waitForJoinPermission() {
+ // TODO https://issues.apache.org/jira/browse/IGNITE-15114
+ }
+
+ /**
+ * Listens Metastorage revision updates.
+ *
+ * @return Future, which completes when the local metadata enough closer
to distributed.
+ */
+ private CompletableFuture<Void> listenMetastorageRevision() {
+ //TODO: IGNITE-15114 This is a temporary solution until full process
of the node join is implemented.
+ if (!metaStorageMgr.isMetaStorageInitializedOnStart()) {
+ return CompletableFuture.completedFuture(null);
+ }
+
+ CompletableFuture<Void> upToDateMetastorageRevisionFut = new
CompletableFuture<>();
+
+ ConfigurationStorageRevisionListener listener = cfgUpdateRevision -> {
+ long metastorageRevision = metaStorageMgr.revision().join();
Review comment:
Despite the fact that it'll rather be storageRevision directValue than
metaStorageMgr.**revision** let's only recheck it on match rather than on every
listener notification.
##########
File path:
modules/metastorage-server/src/main/java/org/apache/ignite/internal/metastorage/server/KeyValueStorage.java
##########
@@ -41,6 +41,13 @@
*/
long revision();
+ /**
+ * Returns the earliest storage revision that is available.
+ *
+ * @return Storage revision.
+ */
+ long earliestRevision();
Review comment:
Because currently there won't be any usages of such method, let's remove
it from given PR for now, what do you think?
##########
File path:
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java
##########
@@ -325,6 +336,10 @@ public void start(@Nullable String cfg) {
nodeCfgMgr.configurationRegistry().initializeDefaults();
}
+ if (!standaloneMode()) {
Review comment:
Seems that currently we're far-far away from any understanding of what
standalone mode is. It might be somehow related to maintenance mode, or not,
it's not clear yet. I'd remove this method until clarification.
##########
File path:
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java
##########
@@ -164,7 +167,121 @@ public void nodeWithDataTest(TestInfo testInfo) {
+ " }\n"
+ "}", workDir);
- TableDefinition scmTbl1 = SchemaBuilders.tableBuilder("PUBLIC",
TABLE_NAME).columns(
+ createTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+
+ IgnitionManager.stop(nodeName);
+
+ ignite = IgnitionManager.start(nodeName, null, workDir);
+
+ checkTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+
+ IgnitionManager.stop(nodeName);
Review comment:
Minor: I'd rather move all after test cleanup logic, e.g.
IgnitionManager.stop() to @afterEach.
##########
File path:
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java
##########
@@ -363,6 +382,98 @@ public void start(@Nullable String cfg) {
}
}
+ /**
+ * Whether the node had started in standalone mode (for example,
maintenance mode).
+ *
+ * @return Whether the node had started in standalone mode.
+ */
+ private boolean standaloneMode() {
+ return false;
+ }
+
+ /**
+ * Awaits for a permission to join the cluster, i.e. node join response
from Cluster Management group.
+ * After the completion of this method, the node is considered as
validated.
+ */
+ private void waitForJoinPermission() {
+ // TODO https://issues.apache.org/jira/browse/IGNITE-15114
+ }
+
+ /**
+ * Listens Metastorage revision updates.
+ *
+ * @return Future, which completes when the local metadata enough closer
to distributed.
+ */
+ private CompletableFuture<Void> listenMetastorageRevision() {
+ //TODO: IGNITE-15114 This is a temporary solution until full process
of the node join is implemented.
+ if (!metaStorageMgr.isMetaStorageInitializedOnStart()) {
+ return CompletableFuture.completedFuture(null);
+ }
+
+ CompletableFuture<Void> upToDateMetastorageRevisionFut = new
CompletableFuture<>();
+
+ ConfigurationStorageRevisionListener listener = cfgUpdateRevision -> {
+ long metastorageRevision = metaStorageMgr.revision().join();
+
+ assert metastorageRevision >= cfgUpdateRevision :
IgniteStringFormatter.format(
+ "Metastorage revision must be greater than local node
applied revision [msRev={}, appliedRev={}",
+ metastorageRevision, cfgUpdateRevision);
+
+ if (isMetadataUpToDate(metastorageRevision, cfgUpdateRevision)) {
+ upToDateMetastorageRevisionFut.complete(null);
+ }
+
+ return CompletableFuture.completedFuture(null);
+ };
+
+
clusterCfgMgr.configurationRegistry().listenUpdateStorageRevision(listener);
Review comment:
As was discussed let's consider configuration data as an only recovery
source and remove corresponding meta storage methods and events that were
introduced: revision, latestRevision, MetastorageEvent.REVISION_APPLIED, etc.
##########
File path:
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java
##########
@@ -164,7 +167,121 @@ public void nodeWithDataTest(TestInfo testInfo) {
+ " }\n"
+ "}", workDir);
- TableDefinition scmTbl1 = SchemaBuilders.tableBuilder("PUBLIC",
TABLE_NAME).columns(
+ createTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+
+ IgnitionManager.stop(nodeName);
+
+ ignite = IgnitionManager.start(nodeName, null, workDir);
+
+ checkTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+
+ IgnitionManager.stop(nodeName);
+ }
+
+ /**
+ * Starts two nodes and checks that the data are storing through restarts.
+ * Nodes restart in the same order when they started at first.
+ *
+ * @param testInfo Test information object.
+ */
+ @Test
+ public void testTwoNodesRestartDirect(TestInfo testInfo) {
+ twoNodesRestart(testInfo, true);
+ }
+
+ /**
+ * Starts two nodes and checks that the data are storing through restarts.
+ * Nodes restart in reverse order when they started at first.
+ *
+ * @param testInfo Test information object.
+ */
+ @Test
+ @Disabled("IGNITE-16034 Unblock a node start that happenes before
Metastorage is ready")
+ public void testTwoNodesRestartReverse(TestInfo testInfo) {
+ twoNodesRestart(testInfo, false);
+ }
+
+ /**
+ * Starts two nodes and checks that the data are storing through restarts.
+ *
+ * @param testInfo Test information object.
+ * @param directOrder When the parameter is true, nodes restart in direct
order, otherwise they restart in reverse order.
+ */
+ private void twoNodesRestart(TestInfo testInfo, boolean directOrder) {
+ String metastorageNode = testNodeName(testInfo, 3344);
+
+ Ignite ignite = IgnitionManager.start(metastorageNode, "{\n"
+ + " \"node\": {\n"
+ + " \"metastorageNodes\":[ " + metastorageNode + " ]\n"
+ + " },\n"
+ + " \"network\": {\n"
+ + " \"port\":3344,\n"
+ + " \"nodeFinder\": {\n"
+ + " \"netClusterNodes\":[ \"localhost:3344\" ] \n"
+ + " }\n"
+ + " }\n"
+ + "}", workDir.resolve(metastorageNode));
+
+ String nodeName = testNodeName(testInfo, 3345);
+
+ IgnitionManager.start(nodeName, "{\n"
+ + " \"node\": {\n"
+ + " \"metastorageNodes\":[ " + metastorageNode + " ]\n"
+ + " },\n"
+ + " \"network\": {\n"
+ + " \"port\":3345,\n"
+ + " \"nodeFinder\": {\n"
+ + " \"netClusterNodes\":[ \"localhost:3344\" ] \n"
+ + " }\n"
+ + " }\n"
+ + "}", workDir.resolve(nodeName));
+
+ createTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+ createTableWithData(ignite, TABLE_NAME_2, i -> "val " + i);
+
+ IgnitionManager.stop(metastorageNode);
+ IgnitionManager.stop(nodeName);
+
+ if (directOrder) {
+ IgnitionManager.start(metastorageNode, null,
workDir.resolve(metastorageNode));
+ ignite = IgnitionManager.start(nodeName, null,
workDir.resolve(nodeName));
+ } else {
+ ignite = IgnitionManager.start(nodeName, null,
workDir.resolve(nodeName));
+ IgnitionManager.start(metastorageNode, null,
workDir.resolve(metastorageNode));
+ }
+
+ checkTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+ checkTableWithData(ignite, TABLE_NAME_2, i -> "val " + i);
+
+ IgnitionManager.stop(metastorageNode);
+ IgnitionManager.stop(nodeName);
+ }
+
+ /**
+ * Checks the table exists and validates all data in it.
+ *
+ * @param ignite Ignite.
+ * @param valueProducer Producer to predict a value.
Review comment:
Minor: let's either add all parameters or none? same for
createTableWithData()
##########
File path:
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteNodeRestartTest.java
##########
@@ -164,7 +167,121 @@ public void nodeWithDataTest(TestInfo testInfo) {
+ " }\n"
+ "}", workDir);
- TableDefinition scmTbl1 = SchemaBuilders.tableBuilder("PUBLIC",
TABLE_NAME).columns(
+ createTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+
+ IgnitionManager.stop(nodeName);
+
+ ignite = IgnitionManager.start(nodeName, null, workDir);
+
+ checkTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+
+ IgnitionManager.stop(nodeName);
+ }
+
+ /**
+ * Starts two nodes and checks that the data are storing through restarts.
+ * Nodes restart in the same order when they started at first.
+ *
+ * @param testInfo Test information object.
+ */
+ @Test
+ public void testTwoNodesRestartDirect(TestInfo testInfo) {
+ twoNodesRestart(testInfo, true);
+ }
+
+ /**
+ * Starts two nodes and checks that the data are storing through restarts.
+ * Nodes restart in reverse order when they started at first.
+ *
+ * @param testInfo Test information object.
+ */
+ @Test
+ @Disabled("IGNITE-16034 Unblock a node start that happenes before
Metastorage is ready")
+ public void testTwoNodesRestartReverse(TestInfo testInfo) {
+ twoNodesRestart(testInfo, false);
+ }
+
+ /**
+ * Starts two nodes and checks that the data are storing through restarts.
+ *
+ * @param testInfo Test information object.
+ * @param directOrder When the parameter is true, nodes restart in direct
order, otherwise they restart in reverse order.
+ */
+ private void twoNodesRestart(TestInfo testInfo, boolean directOrder) {
+ String metastorageNode = testNodeName(testInfo, 3344);
+
+ Ignite ignite = IgnitionManager.start(metastorageNode, "{\n"
+ + " \"node\": {\n"
+ + " \"metastorageNodes\":[ " + metastorageNode + " ]\n"
+ + " },\n"
+ + " \"network\": {\n"
+ + " \"port\":3344,\n"
+ + " \"nodeFinder\": {\n"
+ + " \"netClusterNodes\":[ \"localhost:3344\" ] \n"
+ + " }\n"
+ + " }\n"
+ + "}", workDir.resolve(metastorageNode));
+
+ String nodeName = testNodeName(testInfo, 3345);
+
+ IgnitionManager.start(nodeName, "{\n"
+ + " \"node\": {\n"
+ + " \"metastorageNodes\":[ " + metastorageNode + " ]\n"
+ + " },\n"
+ + " \"network\": {\n"
+ + " \"port\":3345,\n"
+ + " \"nodeFinder\": {\n"
+ + " \"netClusterNodes\":[ \"localhost:3344\" ] \n"
+ + " }\n"
+ + " }\n"
+ + "}", workDir.resolve(nodeName));
+
+ createTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+ createTableWithData(ignite, TABLE_NAME_2, i -> "val " + i);
+
+ IgnitionManager.stop(metastorageNode);
+ IgnitionManager.stop(nodeName);
+
+ if (directOrder) {
+ IgnitionManager.start(metastorageNode, null,
workDir.resolve(metastorageNode));
+ ignite = IgnitionManager.start(nodeName, null,
workDir.resolve(nodeName));
+ } else {
+ ignite = IgnitionManager.start(nodeName, null,
workDir.resolve(nodeName));
+ IgnitionManager.start(metastorageNode, null,
workDir.resolve(metastorageNode));
+ }
+
+ checkTableWithData(ignite, TABLE_NAME, i -> "name " + i);
+ checkTableWithData(ignite, TABLE_NAME_2, i -> "val " + i);
+
+ IgnitionManager.stop(metastorageNode);
+ IgnitionManager.stop(nodeName);
+ }
+
+ /**
+ * Checks the table exists and validates all data in it.
+ *
+ * @param ignite Ignite.
+ * @param valueProducer Producer to predict a value.
+ */
+ private void checkTableWithData(Ignite ignite, String name,
IntFunction<String> valueProducer) {
Review comment:
Why do we need valueProducer? Seems that actual value is partially
generated inside createTableWithData, so what's the point in generating value
prefix externally?
##########
File path:
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/SqlSchemaManagerImpl.java
##########
@@ -133,10 +131,10 @@ public IgniteTable tableById(UUID id, int ver) {
* @param causalityToken Causality token.
*/
public synchronized void onSchemaCreated(String schemaName, long
causalityToken) {
- schemasVv.update(
+ Map<String, IgniteSchema> schemasMap = schemasVv.update(
causalityToken,
schemas -> {
- Map<String, IgniteSchema> res = new HashMap<>(schemas);
+ Map<String, IgniteSchema> res = new HashMap<>(schemas);
Review comment:
2 white spaces?
##########
File path:
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java
##########
@@ -415,6 +325,110 @@ private void
onTableCreateInternal(ConfigurationNotificationEvent<TableView> ctx
defaultDataRegion.start();
}
+ /**
+ * Internal method to create a schema.
+ *
+ * @param schemasCtx Create schema configuration event.
+ */
+ private void
createSchemaInternal(ConfigurationNotificationEvent<SchemaView> schemasCtx) {
+ ExtendedTableConfiguration tblCfg = (ExtendedTableConfiguration)
schemasCtx.config(TableConfiguration.class);
+
+ UUID tblId = tblCfg.id().value();
+
+ long causalityToken = schemasCtx.storageRevision();
+
+ SchemaDescriptor schemaDescriptor =
SchemaSerializerImpl.INSTANCE.deserialize((schemasCtx.newValue().schema()));
+
+ tablesByIdVv.update(causalityToken, tablesById -> {
+ TableImpl table = tablesById.get(tblId);
+
+ ((SchemaRegistryImpl)
table.schemaView()).onSchemaRegistered(schemaDescriptor);
+
+ if (schemaDescriptor.version() != INITIAL_SCHEMA_VERSION) {
+ fireEvent(TableEvent.ALTER, new
TableEventParameters(causalityToken, table), null);
+ }
+
+ return tablesById;
+ }, th -> {
+ throw new
IgniteInternalException(IgniteStringFormatter.format("Cannot create a schema
for table"
+ + " [tableId={}, schemaVer={}]", tblId,
schemaDescriptor.version()), th);
+ });
+ }
+
+ /**
+ * Updates or creates partition raft groups.
+ *
+ * @param assignmentsCtx Change assignment event.
+ */
+ private void
updateAssignmentInternal(ConfigurationNotificationEvent<byte[]> assignmentsCtx)
{
Review comment:
Well, rebalance will change given logic, so let's consider it's fine for
now.
--
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]
