alex-plekhanov commented on a change in pull request #9317:
URL: https://github.com/apache/ignite/pull/9317#discussion_r691970123
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/GridCacheOffheapManager.java
##########
@@ -271,6 +273,12 @@ public IndexStorage getIndexStorage() {
/** {@inheritDoc} */
@Override public void beforeCheckpointBegin(Context ctx) throws
IgniteCheckedException {
+ List<CacheDataStore> destroyedStores =
StreamSupport.stream(cacheDataStores().spliterator(), false)
Review comment:
1. If you do some actions only to check assertions, it's better to wrap
it to `if (U.assertioEnabled()) ...`
2. In this case `assert F.size(cacheDataStores().iterator(),
CacheDataStore::destroyed) == 0;` looks simpler to understand and do not
produce extra objects.
3. Why do we need this check at all, if `cacheDataStores()` doesn't return
destroyed partitions?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheOffheapManagerImpl.java
##########
@@ -398,33 +373,46 @@ public CacheDataStore dataStore(int part) {
/** {@inheritDoc} */
@Override public long cacheEntriesCount(int cacheId, int part) {
- CacheDataStore store = partitionData(part);
+ CacheDataStore store = dataStore(part, true);
return store == null ? 0 : store.cacheSize(cacheId);
}
+ /** {@inheritDoc} */
+ @Override public Iterable<CacheDataStore> cacheDataStores() {
+ return cacheDataStores(grp -> new AlwaysTruePredicate<>());
+ }
+
+ /**
+ * @param factory Factory which will produce filtering predicate.
Review comment:
Why do we need a factory of predicates here? Function argument `grp` is
the same cache group as stored in this offheap manager. We can pass only
predicate with the same behavior.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheOffheapManagerImpl.java
##########
@@ -398,33 +373,46 @@ public CacheDataStore dataStore(int part) {
/** {@inheritDoc} */
@Override public long cacheEntriesCount(int cacheId, int part) {
- CacheDataStore store = partitionData(part);
+ CacheDataStore store = dataStore(part, true);
return store == null ? 0 : store.cacheSize(cacheId);
}
+ /** {@inheritDoc} */
+ @Override public Iterable<CacheDataStore> cacheDataStores() {
+ return cacheDataStores(grp -> new AlwaysTruePredicate<>());
Review comment:
` new AlwaysTruePredicate<>()` -> `F.alwaysTrue()`
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/GridCacheDatabaseSharedManager.java
##########
@@ -2699,10 +2699,9 @@ private RestoreLogicalState applyLogicalUpdates(
CacheGroupContext ctx =
cctx.cache().cacheGroup(rbRec.groupId());
if (ctx != null && !ctx.isLocal()) {
-
ctx.topology().forceCreatePartition(rbRec.partitionId());
+ GridDhtLocalPartition part =
ctx.topology().forceCreatePartition(rbRec.partitionId());
-
ctx.offheap().onPartitionInitialCounterUpdated(rbRec.partitionId(),
rbRec.start(),
- rbRec.range());
+
ctx.offheap().dataStore(part).updateCounter(rbRec.start(), rbRec.range());
Review comment:
`onPartitionInitialCounterUpdated` was equivalent for
`updateInitialCounter`, but why `updateCounter` here?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheOffheapManagerImpl.java
##########
@@ -1280,22 +1239,21 @@ private long allocateForTree() throws
IgniteCheckedException {
/** {@inheritDoc} */
@Override public final CacheDataStore createCacheDataStore(int p) throws
IgniteCheckedException {
- CacheDataStore dataStore;
-
partStoreLock.lock(p);
try {
- assert !partDataStores.containsKey(p);
+ CacheDataStore store = createCacheDataStore0(p);
- dataStore = createCacheDataStore0(p);
+ // Inject row cache cleaner on store creation.
+ // Used in case the cache with enabled SqlOnheapCache is single
cache at the cache group.
+ if (ctx.kernalContext().query().moduleEnabled() && !grp.isLocal())
+
store.setRowCacheCleaner(ctx.kernalContext().indexProcessor().rowCacheCleaner(grp.groupId()));
Review comment:
Why this part was moved from `GridDhtLocalPartition`? I see the only
difference: now row cache cleaner is set under the lock, but looks like this
lock doesn't protect anything related to row cache cleaner.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/pagemem/store/IgnitePageStoreManager.java
##########
@@ -100,7 +100,7 @@ public void initializeForCache(CacheGroupDescriptor
grpDesc, StoredCacheData cac
* @param tag Partition tag (growing 1-based partition file version).
* @throws IgniteCheckedException If failed to handle partition destroy
callback.
*/
- public void onPartitionDestroyed(int grpId, int partId, int tag) throws
IgniteCheckedException;
+ public void truncate(int grpId, int partId, int tag) throws
IgniteCheckedException;
Review comment:
What's wrong with the old method name? It's consistent with
`onPartitionCreated`, can we keep it?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/GridCacheOffheapManager.java
##########
@@ -1290,18 +1271,18 @@ private Metas getOrAllocateCacheMetas() throws
IgniteCheckedException {
}
/** {@inheritDoc} */
- @Override public void preloadPartition(int part) throws
IgniteCheckedException {
+ @Override public void preloadPartition(int partId) throws
IgniteCheckedException {
if (grp.isLocal()) {
- dataStore(part).preload();
+ dataStore(null).preload();
return;
}
- GridDhtLocalPartition locPart = grp.topology().localPartition(part,
AffinityTopologyVersion.NONE, false, false);
+ GridDhtLocalPartition locPart = grp.topology().localPartition(partId,
AffinityTopologyVersion.NONE, false, false);
assert locPart != null && locPart.reservations() > 0;
- locPart.dataStore().preload();
+ dataStore(locPart).preload();
Review comment:
I think this change is redundant (it's an additional check in
`dataStore()` for local caches, but we already checked it)
##########
File path:
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/query/CacheScanQueryFailoverTest.java
##########
@@ -172,7 +173,7 @@ public void testScanQueryOnEvictedPartition() throws
Exception {
// Force checkpoint to destroy evicted partitions store.
forceCheckpoint(grid0);
- GridTestUtils.assertThrowsAnyCause(log, iter1::next,
IgniteException.class, "Failed to get next data row");
+ GridTestUtils.assertThrows(log, iter1::next,
NoSuchElementException.class, null);
Review comment:
`assertFalse(iter1.hasNext());`
##########
File path:
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerClusterByClassTest.java
##########
@@ -914,7 +914,7 @@ public void
testCacheIdleVerifyDumpForCorruptedDataOnSystemCache() throws Except
U.log(log, dumpWithConflicts);
// Non-persistent caches do not have counter conflicts
- assertContains(log, dumpWithConflicts, "found 3 conflict
partitions: [counterConflicts=1, " +
+ assertContains(log, dumpWithConflicts, "found 4 conflict
partitions: [counterConflicts=2, " +
Review comment:
Why count of conflicts changed? I didn't get how it related to changes
made.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheOffheapManagerImpl.java
##########
@@ -398,33 +373,46 @@ public CacheDataStore dataStore(int part) {
/** {@inheritDoc} */
@Override public long cacheEntriesCount(int cacheId, int part) {
- CacheDataStore store = partitionData(part);
+ CacheDataStore store = dataStore(part, true);
return store == null ? 0 : store.cacheSize(cacheId);
}
+ /** {@inheritDoc} */
+ @Override public Iterable<CacheDataStore> cacheDataStores() {
+ return cacheDataStores(grp -> new AlwaysTruePredicate<>());
+ }
+
+ /**
+ * @param factory Factory which will produce filtering predicate.
+ * @return Iterable over all existing cache data stores except which one
is marked as <tt>destroyed</tt>.
+ */
+ private Iterable<CacheDataStore> cacheDataStores(
+ Function<CacheGroupContext, IgnitePredicate<GridDhtLocalPartition>>
factory
+ ) {
+ return grp.isLocal() ? Collections.singletonList(locCacheDataStore) :
+ F.iterator(grp.topology().currentLocalPartitions(),
GridDhtLocalPartition::dataStore, true,
+ factory.apply(grp), p -> !p.dataStore().destroyed());
+ }
+
/**
* @param primary Primary data flag.
- * @param backup Primary data flag.
+ * @param backup Backup data flag.
* @param topVer Topology version.
* @return Data stores iterator.
*/
private Iterator<CacheDataStore> cacheData(boolean primary, boolean
backup, AffinityTopologyVersion topVer) {
assert primary || backup;
- if (grp.isLocal())
- return singletonIterator(locCacheDataStore);
- else {
- Iterator<GridDhtLocalPartition> it =
grp.topology().currentLocalPartitions().iterator();
-
+ return cacheDataStores(grp -> {
if (primary && backup)
- return F.iterator(it, GridDhtLocalPartition::dataStore, true);
+ return new AlwaysTruePredicate<>();
Review comment:
`F.alwaysTrue()`
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheOffheapManagerImpl.java
##########
@@ -624,7 +612,7 @@ public CacheDataStore dataStore(int part) {
/** {@inheritDoc} */
@Nullable @Override public CacheDataRow read(GridCacheContext cctx,
KeyCacheObject key)
throws IgniteCheckedException {
- CacheDataStore dataStore = dataStore(cctx, key);
+ CacheDataStore dataStore = dataStore(cctx.affinity().partition(key),
false);
Review comment:
Are you sure that the call of `cctx.affinity().partition(key)` for local
cache doesn't throw an exception (for example, if affinity assignment cache is
not set for local caches)? I think it's better to keep the old method (with
local cache check).
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheOffheapManagerImpl.java
##########
@@ -310,26 +308,28 @@ private void removeCacheData(int cacheId) {
}
}
- /**
- * @param part Partition.
- * @return Data store for given entry.
- */
- @Override public CacheDataStore dataStore(GridDhtLocalPartition part) {
+ /** {@inheritDoc} */
+ @Override public CacheDataStore dataStore(@Nullable GridDhtLocalPartition
part) {
if (grp.isLocal())
return locCacheDataStore;
- else {
- assert part != null;
- return part.dataStore();
- }
+ assert part != null;
+
+ return part.dataStore();
}
/**
- * @param part Partition.
- * @return Data store for given entry.
+ * @param partId Partition id.
+ * @param renting {@code true} if renting partitions must also be shown.
Review comment:
I think a name like `includeRenting` is more appropriate.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheOffheapManagerImpl.java
##########
@@ -1546,6 +1498,11 @@ public CacheDataStoreImpl(
mvccUpdateMarker = new MvccMarkUpdatedHandler(grp);
mvccUpdateTxStateHint = new MvccUpdateTxStateHintHandler(grp);
mvccApplyChanges = new MvccApplyChangesHandler(grp);
+
+ if (cleaner == null)
Review comment:
What is the purpose of change row cache cleaner to a supplier? The code
complexity is about the same, the amount of code is about the same, the
behavior is about the same.
--
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]
