sergey-chugunov-1985 commented on code in PR #12133:
URL: https://github.com/apache/ignite/pull/12133#discussion_r2149627925
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/filename/CacheConfigStoragePathTest.java:
##########
@@ -156,18 +193,27 @@ else if (storage == null)
snpRootF.apply(storagePath(STORAGE_PATH_2))
));
- assertEquals(path != null ? 1 : 3, roots.size());
+ boolean idxPathUsed = idxStorage && idxPartMustExistsInSnapshot();
+
+ if (idxPathUsed)
+ roots.add(snpRootF.apply(storagePath(IDX_PATH)));
+
+ // Sanity check storagePath returns different paths.
+ assertEquals(path != null ? 1 : (idxPathUsed ? 4 : 3), roots.size());
// Root -> cache -> partition set.
Map<File, Map<String, Set<Integer>>> snpFiles = new HashMap<>();
// Collecting all partition files under each snapshot root.
- roots.forEach(snpRoot -> {
+ for (File snpRoot : roots) {
assertTrue(snpRoot.exists());
assertTrue(snpRoot.isDirectory());
+ Predicate<Path> pathPredicate = p ->
NodeFileTree.partitionFile(p.toFile())
Review Comment:
Side note. What do you think about renaming `NodeFileTree.partitionFile`
into `NodeFileTree.isPartitionFile`? Not necessarily in this PR but in general.
My reason for that: when reviewing this PR I had to consult with source code
to figure out that semantics of `partitionFile` and `partitionFileName` is very
different despite their names are rather similar. Name like `isPartitionFile`
immediately reveals that this method is for pattern matching and not for
constructing a new File from an old one.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/filename/FileTreeUtils.java:
##########
@@ -78,6 +88,23 @@ public static void removeTmpSnapshotFiles(SnapshotFileTree
sft, boolean err, Ign
return resolveStorage(csp, part);
}
+ /**
+ * @param dsCfg Data storage configuration.
+ * @return All known storages.
+ * @throws IgniteCheckedException
+ */
+ public static Set<String> nodeStorages(@Nullable DataStorageConfiguration
dsCfg) throws IgniteCheckedException {
Review Comment:
I think using `@Nullable` is misleading here as the method itself doesn't
handle null values "normally" but throws exception instead.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/filename/NodeFileTree.java:
##########
@@ -640,19 +656,35 @@ public File
tmpCacheConfigurationFile(CacheConfiguration<?, ?> ccfg) {
* @return Partition file.
*/
public File partitionFile(CacheConfiguration<?, ?> ccfg, int part) {
- return new File(resolveStorage(cacheStorages(ccfg), part),
partitionFileName(part));
+ if (part == INDEX_PARTITION) {
+ String idxPath = ccfg.getIndexPath();
+
+ if (!F.isEmpty(idxPath))
+ return new File(cacheStorage(idxPath, cacheDirName(ccfg)),
partitionFileName(INDEX_PARTITION));
+ }
+
+ return new File(resolveStorage(cacheStorages(ccfg, false), part),
partitionFileName(part));
Review Comment:
Side note (don't feel obligated to fix it right now or at all). It seems to
me that we have somewhat disorienting naming of two methods here,
`cacheStorage` and `resolveStorage` - one private and one static from utils
class. They look very similar to each other, but their semantics is very
different.
If we can come up with better naming, it would be great. I'm ready to
participate in a brainstorm when I get more familiar with this class and its
neighbours.
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/filename/CacheConfigStoragePathTest.java:
##########
@@ -75,6 +79,39 @@ public class CacheConfigStoragePathTest extends
AbstractDataRegionRelativeStorag
};
}
+ /** Sanity checks - all pathes for all partitions are different and
contains cacheDir. */
Review Comment:
Minor notes: `paths` instead of `pathes`, `contain` instead of `contains`
(as subject is in plural form).
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/filename/NodeFileTree.java:
##########
@@ -881,9 +925,10 @@ else if (name.equals(METASTORAGE_DIR_NAME))
* @param storagePath Value from config.
* @return File storage.
* @see CacheConfiguration#getStoragePaths()
+ * @see CacheConfiguration#getIndexPath()
*/
- private File cacheStorageRoot(@Nullable String storagePath) {
- return storagePath == null ? nodeStorage :
extraStorages.getOrDefault(storagePath, nodeStorage);
+ private File cacheStorage(@Nullable String storagePath, String
cacheDirName) {
Review Comment:
I suggest to add javadoc for new parameter `cacheDirName`. As far as I can
see it is a subdirectory name under `storagePath` parent directory, but it is
not possible to deduce this fact from param name itself.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/filename/FileTreeUtils.java:
##########
@@ -70,6 +75,11 @@ public static void removeTmpSnapshotFiles(SnapshotFileTree
sft, boolean err, Ign
* @return Storage path from config for partition.
*/
public static @Nullable String partitionStorage(CacheConfiguration<?, ?>
ccfg, int part) {
+ if (part == INDEX_PARTITION) {
+ if (F.isEmpty(ccfg.getIndexPath()))
Review Comment:
Maybe we should use `!F.isEmpty(...)` condition here? Because right now
logic looks counter-intuitive: if index path is not set, return index path.
If this comment makes sense I think we should add a test for it.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/filename/NodeFileTree.java:
##########
Review Comment:
Side note (don't feel obligated to fix it right now or at all). One
particular feature of this class makes its review a challenging exercise: there
are pairs of methods named like `cacheStorage` and `cacheStorages` with
overloaded versions on top of that. Difference in name in just one letter makes
it quite hard to navigate the context and I'm afraid error-prone too.
If we can rename more specific methods (by this I mean `cacheStorage` here)
to something `storageForCache` without compromizing its semantics, I believe it
would simplify reading and modifying this class in the future.
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
