sergey-chugunov-1985 commented on code in PR #12081:
URL: https://github.com/apache/ignite/pull/12081#discussion_r2135190452
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/FilePageStoreManager.java:
##########
@@ -192,19 +192,20 @@ public FilePageStoreManager(GridKernalContext ctx) {
/** {@inheritDoc} */
@Override public void cleanupPersistentSpace(CacheConfiguration
cacheConfiguration) throws IgniteCheckedException {
try {
- File cacheWorkDir = ft.cacheStorage(cacheConfiguration);
-
- if (!cacheWorkDir.exists())
- return;
-
- try (DirectoryStream<Path> files =
newDirectoryStream(cacheWorkDir.toPath(),
- new DirectoryStream.Filter<Path>() {
- @Override public boolean accept(Path entry) throws
IOException {
- return NodeFileTree.binFile(entry.toFile());
+ for (File cacheWorkDir : ft.cacheStorages(cacheConfiguration)) {
Review Comment:
I suggest to move try-catch block inside the for loop, this will allow us to
throw more precise exception if cleanup of a specific directory fails.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/FilePageStoreManager.java:
##########
@@ -627,47 +628,47 @@ private CacheStoreHolder initDir(
* @param cft Cache file tree.
*/
public static boolean checkAndInitCacheWorkDir(CacheFileTree cft) throws
IgniteCheckedException {
- File cacheWorkDir = cft.storage();
-
boolean dirExisted = false;
- ReadWriteLock lock =
initDirLock.getLock(cacheWorkDir.getName().hashCode());
+ for (File cacheWorkDir : cft.storages()) {
+ ReadWriteLock lock =
initDirLock.getLock(cacheWorkDir.getName().hashCode());
- lock.writeLock().lock();
+ lock.writeLock().lock();
- try {
- if (!Files.exists(cacheWorkDir.toPath())) {
- try {
- Files.createDirectory(cacheWorkDir.toPath());
- }
- catch (IOException e) {
- throw new IgniteCheckedException("Failed to initialize
cache working directory " +
- "(failed to create, make sure the work folder has
correct permissions): " +
- cacheWorkDir.getAbsolutePath(), e);
+ try {
+ if (!Files.exists(cacheWorkDir.toPath())) {
+ try {
+ Files.createDirectory(cacheWorkDir.toPath());
+ }
+ catch (IOException e) {
+ throw new IgniteCheckedException("Failed to initialize
cache working directory " +
+ "(failed to create, make sure the work folder has
correct permissions): " +
+ cacheWorkDir.getAbsolutePath(), e);
+ }
}
- }
- else {
- if (cacheWorkDir.isFile())
- throw new IgniteCheckedException("Failed to initialize
cache working directory " +
- "(a file with the same name already exists): " +
cacheWorkDir.getAbsolutePath());
+ else {
+ if (cacheWorkDir.isFile())
+ throw new IgniteCheckedException("Failed to initialize
cache working directory " +
+ "(a file with the same name already exists): " +
cacheWorkDir.getAbsolutePath());
- Path cacheWorkDirPath = cacheWorkDir.toPath();
+ Path cacheWorkDirPath = cacheWorkDir.toPath();
- Path tmp =
cacheWorkDirPath.getParent().resolve(cacheWorkDir.getName() + TMP_SUFFIX);
+ Path tmp =
cacheWorkDirPath.getParent().resolve(cacheWorkDir.getName() + TMP_SUFFIX);
- dirExisted = true;
+ dirExisted = true;
Review Comment:
I suspect this flag is excessive and only confuses logic of the method. From
what I see the method either checks/creates necessary directory or throws an
exception if something goes wrong. If this is true we can safely remove the
flag and replace it with `return true` at the end.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/filename/CacheFileTree.java:
##########
@@ -156,7 +157,7 @@ public File defragmentedIndexTmpFile() {
* @see #defragmentedIndexTmpFile()
*/
public File defragmentedIndexFile() {
- return new File(storage(), DFRG_INDEX_FILE_NAME);
+ return new File(partitionRoot(INDEX_PARTITION), DFRG_INDEX_FILE_NAME);
Review Comment:
What do you think about an idea to allow user to explicitly specify a
separate storage path for `index.bin`? If `index.bin` doesn't share a storage
device with regular partition files, performance of index operations won't be
affected by rw operations on partitions.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/filename/FileTreeUtils.java:
##########
@@ -61,6 +63,20 @@ public static void removeTmpSnapshotFiles(SnapshotFileTree
sft, boolean err, Ign
removeTmpDir(tmpDrStorage.getParentFile(), err, log);
}
+ /**
+ * @param ccfg Cache configuration.
+ * @param part Partition.
+ * @return Storage path from config for partition.
+ */
+ public static @Nullable String partitionStorage(CacheConfiguration<?, ?>
ccfg, int part) {
+ String[] csp = ccfg.getStoragePaths();
+
+ if (csp == null)
Review Comment:
I would add an array empty check to make sure we won't perform division by
zero down the code flow.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/file/FilePageStoreManager.java:
##########
@@ -298,7 +299,7 @@ public FilePageStoreManager(GridKernalContext ctx) {
new
MaintenanceTask(CORRUPTED_DATA_FILES_MNTC_TASK_NAME,
"Corrupted cache groups found",
cacheCfgs.stream()
- .map(ccfg -> ft.cacheStorage(ccfg).getName())
+ .map(ccfg ->
ft.cacheStorages(ccfg)[0].getName())
Review Comment:
I don't like this magic number: it forces a person reading the code to
remember (or keep in mind) why the first element of the array is special.
What do you think about introducing a separate method in `NodeFileTree` to
hide this detail? We could name the method like `cacheInternalConfigStorage` or
other name capturing semantics of a directory that contains configuration and
management files related to this cache or cache group. This method could be
reused to construct path to cache.dat file with serialized cache configuration.
You can consider this idea as an improvement for future, no need to
implement it right now.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/filename/FileTreeUtils.java:
##########
@@ -100,4 +116,13 @@ private static void createAndCheck(File dir, String desc,
IgniteLogger log) thro
"Current persistence store directory is: [" +
dir.getAbsolutePath() + "]");
}
}
+
+ /**
+ * @param storages Storages to select from.
+ * @param part Partition.
+ * @return Storage for partition.
+ */
+ static <T> T resolveStorage(T[] storages, int part) {
+ return storages[(part + 1) % storages.length];
Review Comment:
Could you please clarify to me why do we additionally increment `part` here?
Most likely I don't undestand some details, but if part starts from 0, plain
formula `part % storages.length` would work.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/filename/NodeFileTree.java:
##########
Review Comment:
I suspect an update for javadoc here is also needed.
##########
modules/core/src/main/java/org/apache/ignite/configuration/CacheConfiguration.java:
##########
Review Comment:
As far as I remember there is a feature that allows user to override cache
configuration and start a node with a config that differs from what is stored
on disk.
Are you aware of such feature? If the feature indeed exists, do we have
protection against user just reshuffling storage directories without renaming
it or changing their number?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/filename/CacheFileTree.java:
##########
@@ -209,7 +210,17 @@ public File defragmentedPartMappingFile(int partId) {
* @see
DefragmentationFileUtils#batchRenameDefragmentedCacheGroupPartitions(CacheFileTree)
*/
public File defragmentationCompletionMarkerFile() {
- return new File(storage(), DFRG_COMPLETION_MARKER_FILE_NAME);
+ return new File(storages()[0], DFRG_COMPLETION_MARKER_FILE_NAME);
+ }
+
+ /**
+ * @param part Partition index.
+ * @return Root directory. for partition file.
Review Comment:
Excessive period after `directory`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java:
##########
@@ -2769,36 +2781,36 @@ private void checkIncrementalCanBeCreated(
"Encrypted cache groups not supported [groupId=" + grpId +
']');
}
- File snpCacheDir = sft.cacheStorage(gctx.config());
-
- if (!snpCacheDir.exists()) {
- throw new IgniteCheckedException("Create incremental snapshot
request has been rejected. " +
- "Cache group directory not found [groupId=" + grpId + ']');
- }
+ for (File snpCacheDir : sft.cacheStorages(gctx.config())) {
+ if (!snpCacheDir.exists()) {
+ throw new IgniteCheckedException("Create incremental
snapshot request has been rejected. " +
+ "Cache group directory not found [groupId=" + grpId +
']');
Review Comment:
Please add a dir name to exception message, otherwise we won't know what
directory hasn't been found exactly.
--
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]
