[GitHub] [ignite] timoninmaxim commented on a change in pull request #9345: IGNITE-15117 CDC for in-memory caches

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

timoninmaxim commented on a change in pull request #9345:
URL: https://github.com/apache/ignite/pull/9345#discussion_r800645728



##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/cdc/SqlCdcTest.java
##########
@@ -55,14 +60,31 @@
     /** */
     public static final String MSK = "Moscow";
 
+    /** */
+    @Parameterized.Parameter
+    public boolean persistenceEnabled;
+
+    /** */
+    @Parameterized.Parameters(name = "persistence={0}")
+    public static Collection<?> parameters() {
+        List<Object[]> params = new ArrayList<>();
+
+        for (boolean persistenceEnabled : new boolean[] {true, false})
+            params.add(new Object[] {persistenceEnabled});
+
+        return params;
+    }
+
+
     /** {@inheritDoc} */
     @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
         IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
 
         cfg.setDataStorageConfiguration(new DataStorageConfiguration()
-            .setCdcEnabled(true)
             .setWalForceArchiveTimeout(WAL_ARCHIVE_TIMEOUT)
-            .setDefaultDataRegionConfiguration(new DataRegionConfiguration().setPersistenceEnabled(true)));
+            .setDefaultDataRegionConfiguration(new DataRegionConfiguration()
+                .setPersistenceEnabled(true)

Review comment:
       There should be a var `persistenceEnabled` instead of `true`, shouldn't it?

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/cdc/CdcMain.java
##########
@@ -110,7 +111,7 @@
  */
 public class CdcMain implements Runnable {
     /** */
-    public static final String ERR_MSG = "Persistence disabled. Capture Data Change can't run!";
+    public static final String ERR_MSG = "Persistence and CDC disabled. Capture Data Change can't run!";

Review comment:
       Persistance isn't a requirement anymore, is it?

##########
File path: modules/core/src/test/java/org/apache/ignite/cdc/AbstractCdcTest.java
##########
@@ -75,7 +75,7 @@
     public static final int WAL_ARCHIVE_TIMEOUT = 5_000;
 
     /** Keys count. */
-    public static final int KEYS_CNT = 50;
+    public static final int KEYS_CNT = 10;

Review comment:
       Why not 50?

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheUtils.java
##########
@@ -1987,6 +2009,48 @@ public static boolean isPersistenceEnabled(IgniteConfiguration cfg) {
         return isPersistenceEnabled(cfg.getDataStorageConfiguration());
     }
 
+    /**
+     * @return {@code true} if CDC enabled.
+     */
+    public static boolean isCdcEnabled(DataStorageConfiguration cfg) {
+        if (cfg == null)
+            return false;
+
+        DataRegionConfiguration dfltReg = cfg.getDefaultDataRegionConfiguration();
+
+        if (dfltReg != null && dfltReg.isCdcEnabled())
+            return true;
+
+        DataRegionConfiguration[] regCfgs = cfg.getDataRegionConfigurations();
+
+        if (regCfgs == null)
+            return false;
+
+        for (DataRegionConfiguration regCfg : regCfgs) {
+            if (regCfg.isCdcEnabled())
+                return true;
+        }
+
+        return false;
+    }
+
+    /**
+     * @param ctx Group context.
+     * @return {@code True} if {@link DataRecord} should be logged into WAL.
+     */
+    public static boolean isDataRecordsEnabled(CacheGroupContext ctx) {
+        if (ctx.dataRegion() == null)
+            return false;
+
+        if (ctx.persistenceEnabled())
+            return ctx.walEnabled();
+
+        if (ctx.systemCache())
+            return false;
+
+        return ctx.dataRegion().config().isCdcEnabled() && ctx.walEnabled();

Review comment:
       Am I correct that CDC doesn't work without WAL?

##########
File path: modules/core/src/test/java/org/apache/ignite/cdc/CdcCacheVersionTest.java
##########
@@ -81,9 +81,10 @@
         IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
 
         cfg.setDataStorageConfiguration(new DataStorageConfiguration()
-            .setCdcEnabled(true)
             .setWalForceArchiveTimeout(WAL_ARCHIVE_TIMEOUT)
-            .setDefaultDataRegionConfiguration(new DataRegionConfiguration().setPersistenceEnabled(true)));
+            .setDefaultDataRegionConfiguration(new DataRegionConfiguration()
+                .setPersistenceEnabled(true)

Review comment:
       Can we remove persistence here?

##########
File path: modules/indexing/src/test/java/org/apache/ignite/internal/cdc/SqlCdcTest.java
##########
@@ -55,14 +60,31 @@
     /** */
     public static final String MSK = "Moscow";
 
+    /** */
+    @Parameterized.Parameter
+    public boolean persistenceEnabled;
+
+    /** */
+    @Parameterized.Parameters(name = "persistence={0}")
+    public static Collection<?> parameters() {
+        List<Object[]> params = new ArrayList<>();

Review comment:
       full method can be replaced with 
   ```
   return new Object[] {false, true};
   ```
   




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