ibessonov commented on code in PR #831: URL: https://github.com/apache/ignite-3/pull/831#discussion_r886586276
########## examples/src/main/java/org/apache/ignite/example/storage/VolatilePageMemoryStorageExample.java: ########## @@ -0,0 +1,170 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.example.storage; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.Statement; + +/** + * This example demonstrates a usage of the PageMemory storage engine configured with an in-memory data region. + * + * <p>To run the example, do the following: + * <ol> + * <li>Import the examples project into you IDE.</li> + * <li> + * Download and prepare artifacts for run Ignite node using the CLI tool (if not done yet):<br> + * {@code ignite init} + * </li> + * <li> + * Add module "ignite-storage-page-memory" using the CLI tool (if not done yet):<br> Review Comment: Shouldn't it be there by default? ########## examples/src/integrationTest/java/org/apache/ignite/example/AbstractExamplesTest.java: ########## @@ -52,7 +59,16 @@ public void startNode() { IgnitionManager.init(TEST_NODE_NAME, List.of(TEST_NODE_NAME), "cluster"); - assertThat(ignite, willCompleteSuccessfully()); + assertThat(igniteFuture, willCompleteSuccessfully()); + + ignite = (IgniteImpl) igniteFuture.get(1, TimeUnit.MILLISECONDS); Review Comment: You can call "get" without timeout, future is already completed at this point ########## examples/pom.xml: ########## @@ -60,6 +60,11 @@ <artifactId>ignite-client</artifactId> </dependency> + <dependency> Review Comment: I'm against this decision as well, why would you explicitly depend on internal module? I actually need an answer ########## examples/README.md: ########## @@ -10,17 +10,10 @@ The following examples are included: * `KeyValueViewExample` - demonstrates the usage of the `org.apache.ignite.table.KeyValueView` API * `SqlJdbcExample` - demonstrates the usage of the Apache Ignite JDBC driver. * `RebalanceExample` - demonstrates the data rebalancing process. +* `VolatilePageMemoryStorageExample` - demonstrates the usage of the PageMemory storage engine configured with an in-memory data region. -To run the `RebalanceExample`, refer to its JavaDoc for instructions. +Before running the examples, read [../modules/cli/README.md](../modules/cli/README.md). Review Comment: Are you sure that it's a good idea to redirect users into a project with source code? Examples modules is a thing by itself, at least this is what I thought ########## examples/src/main/java/org/apache/ignite/example/rebalance/RebalanceExample.java: ########## @@ -40,17 +40,32 @@ * <ol> * <li>Import the examples project into you IDE.</li> * <li> + * Download and prepare artifacts for run Ignite node using the CLI tool (if not done yet):<br> + * {@code ignite init} + * </li> + * <li> * Start <b>two</b> nodes using the CLI tool:<br> * {@code ignite node start --config=$IGNITE_HOME/examples/config/ignite-config.json my-first-node}<br> * {@code ignite node start --config=$IGNITE_HOME/examples/config/ignite-config.json my-second-node} * </li> + * <li> + * Cluster initialization using the CLI tool (if not done yet):<br> + * {@code ignite cluster init --cluster-name=ignite-cluster --node-endpoint=localhost:10300 --meta-storage-node=my-first-node} + * </li> * <li>Run the example in the IDE.</li> * <li> - * When requested, start another two nodes using the CLI tool: + * When requested, start another <b>two</b> nodes using the CLI tool: * {@code ignite node start --config=$IGNITE_HOME/examples/config/ignite-config.json my-first-additional-node}<br> * {@code ignite node start --config=$IGNITE_HOME/examples/config/ignite-config.json my-second-additional-node} * </li> * <li>Press {@code Enter} to resume the example.</li> + * <li> + * Stop <b>four</b> nodes using the CLI tool:<br> + * {@code ignite node stop --config=$IGNITE_HOME/examples/config/ignite-config.json my-first-node}<br> Review Comment: What's going on here? Why tf would you need to pass a config file when you're stopping a node? ########## examples/src/main/java/org/apache/ignite/example/storage/VolatilePageMemoryStorageExample.java: ########## @@ -0,0 +1,170 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.example.storage; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.Statement; + +/** + * This example demonstrates a usage of the PageMemory storage engine configured with an in-memory data region. + * + * <p>To run the example, do the following: + * <ol> + * <li>Import the examples project into you IDE.</li> + * <li> + * Download and prepare artifacts for run Ignite node using the CLI tool (if not done yet):<br> + * {@code ignite init} + * </li> + * <li> + * Add module "ignite-storage-page-memory" using the CLI tool (if not done yet):<br> + * {@code ignite module add mvn:org.apache.ignite:ignite-storage-page-memory:version}<br> + * For example:<br> + * {@code ignite module add mvn:org.apache.ignite:ignite-storage-page-memory:3.0.0-SNAPSHOT}<br> + * </li> + * <li> + * Start a server node using the CLI tool:<br> + * {@code ignite node start --config=$IGNITE_HOME/examples/config/ignite-config.json my-first-node} + * </li> + * <li> + * Cluster initialization using the CLI tool (if not done yet):<br> + * {@code ignite cluster init --cluster-name=ignite-cluster --node-endpoint=localhost:10300 --meta-storage-node=my-first-node} + * </li> + * <li> + * Add configuration for in-memory data region of of the PageMemory storage engine using the CLI tool (if not done yet):<br> + * {@code ignite config set --type=cluster "{pageMemory:{regions:[{name:in-memory,persistent:false}]}}"} Review Comment: Actually, I would advice to use simplified syntax. Here, it's enough to do the following: `pageMemory.regions.in-memory: { persistent = false }` we don't need that many braces. Also, how common is it to use dashes in names? I don't know ########## examples/src/main/java/org/apache/ignite/example/storage/VolatilePageMemoryStorageExample.java: ########## @@ -0,0 +1,170 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.example.storage; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.Statement; + +/** + * This example demonstrates a usage of the PageMemory storage engine configured with an in-memory data region. + * + * <p>To run the example, do the following: + * <ol> + * <li>Import the examples project into you IDE.</li> + * <li> + * Download and prepare artifacts for run Ignite node using the CLI tool (if not done yet):<br> + * {@code ignite init} + * </li> + * <li> + * Add module "ignite-storage-page-memory" using the CLI tool (if not done yet):<br> + * {@code ignite module add mvn:org.apache.ignite:ignite-storage-page-memory:version}<br> + * For example:<br> + * {@code ignite module add mvn:org.apache.ignite:ignite-storage-page-memory:3.0.0-SNAPSHOT}<br> + * </li> + * <li> + * Start a server node using the CLI tool:<br> + * {@code ignite node start --config=$IGNITE_HOME/examples/config/ignite-config.json my-first-node} Review Comment: We should move configs to HOCON btw, otherwise no one will ever know that we can do so. I'm not sure that we need to do it right now, but eventually - for sure. JSON sucks ########## modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/PageMemoryStorageEngine.java: ########## @@ -64,26 +70,18 @@ public PageMemoryStorageEngine( public void start() { int pageSize = engineConfig.pageSize().value(); - VolatilePageMemoryDataRegion defaultRegion = new VolatilePageMemoryDataRegion( - engineConfig.defaultRegion(), - ioRegistry, pageSize - ); + addDataRegion(DEFAULT_DATA_REGION_NAME, engineConfig.defaultRegion(), pageSize); - defaultRegion.start(); + // TODO: IGNITE-17066 Add handling deleting/updating data regions configuration + engineConfig.regions().listenElements(new ConfigurationNamedListListener<>() { Review Comment: I'm sorry, I don't think that this is correct and that this is what we have discussed privately. This should be done with "any" listeners and they have to be registered in constructor, not during the start. And, as always, everything is synchronous. I'm not asking to change it now, but I see a very dangerous tendency - we don't think "parallel", there's a lot of baggage already in the code that must be redone. -- 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]
