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

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

anton-vinogradov commented on a change in pull request #9345:
URL: https://github.com/apache/ignite/pull/9345#discussion_r799487590



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheMapEntry.java
##########
@@ -6961,7 +6961,7 @@ private void versionCheck(@Nullable IgniteBiTuple<Object, Exception> invokeRes)
             if (cctx.offheap().mvccApplyHistoryIfAbsent(this, entryHist)) {
                 updated = true;
 
-                if (!cctx.isNear() && cctx.group().persistenceEnabled() && cctx.group().walEnabled()) {
+                if (!cctx.isNear() && CU.isDataRecordsEnabled(cctx.group())) {

Review comment:
       What is `isDataRecordsEnabled`, how to guess it's about WAL?

##########
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:
       Why `and` while `or`?
   Should we print proper states here?

##########
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:
       we should have a test with at least 2 data regions, one is with CDC and another is without.

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/binary/BinaryMetadataFileStore.java
##########
@@ -89,10 +89,11 @@
     ) throws IgniteCheckedException {
         this.metadataLocCache = metadataLocCache;
         this.ctx = ctx;
-        this.isPersistenceEnabled = CU.isPersistenceEnabled(ctx.config());
+        this.isPersistenceMetadataEnabled = CU.isPersistenceEnabled(ctx.config())

Review comment:
       Looks like a bad cast. 
   `CU.isPersistenceEnabled` may mean `isPersistenceMetadataEnabled` as well, but we should have special metadata aware method or keep it as is.
   Such cast just adds complexity without pros.

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/cdc/CdcMain.java
##########
@@ -355,7 +356,7 @@ private CdcFileLockHolder lockPds() throws IgniteCheckedException {
             new PdsFolderResolver<>(igniteCfg, log, igniteCfg.getConsistentId(), this::tryLock).resolve();
 
         if (settings == null) {
-            throw new IgniteException("Can't find folder to read WAL segments from based on provided configuration! " +
+            throw new IgniteException("Can't find folder to read WAL segments from! " +

Review comment:
       ... the folder ...
   Could we simplify `folder to read ... from`?

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/FileWriteAheadLogManager.java
##########
@@ -276,9 +281,12 @@
     /** WAL archive directory (including consistent ID as subfolder). */
     private File walArchiveDir;
 
-    /** WAL cdc directory (including consistent ID as subfolder) */
+    /** WAL CDC directory (including consistent ID as subfolder). */
     private File walCdcDir;
 
+    /** CDC enabled flag. */
+    private final boolean cdcEnabled;

Review comment:
       Looks like `walCdcDir != null` means cdcEnabled?




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