Re: [PR] Extend region migration speed limit to transfers and deletion [iotdb]
CRZbulabula merged PR #18118: URL: https://github.com/apache/iotdb/pull/18118 -- 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]
Re: [PR] Extend region migration speed limit to transfers and deletion [iotdb]
sonarqubecloud[bot] commented on PR #18118: URL: https://github.com/apache/iotdb/pull/18118#issuecomment-4921677357 ## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18118) **Quality Gate passed** Issues  [7 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=18118&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=18118&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=18118&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=18118&metric=new_coverage&view=list)  [2.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=18118&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18118) -- 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]
Re: [PR] Extend region migration speed limit to transfers and deletion [iotdb]
sonarqubecloud[bot] commented on PR #18118: URL: https://github.com/apache/iotdb/pull/18118#issuecomment-4916299190 ## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18118) **Quality Gate passed** Issues  [7 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=18118&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=18118&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=18118&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=18118&metric=new_coverage&view=list)  [2.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=18118&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18118) -- 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]
Re: [PR] Extend region migration speed limit to transfers and deletion [iotdb]
sonarqubecloud[bot] commented on PR #18118: URL: https://github.com/apache/iotdb/pull/18118#issuecomment-4915675592 ## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18118) **Quality Gate passed** Issues  [7 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=18118&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=18118&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=18118&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=18118&metric=new_coverage&view=list)  [2.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=18118&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18118) -- 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]
Re: [PR] Extend region migration speed limit to transfers and deletion [iotdb]
CRZbulabula commented on code in PR #18118:
URL: https://github.com/apache/iotdb/pull/18118#discussion_r3544568444
##
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/utils/FileUtils.java:
##
@@ -161,7 +179,16 @@ public static void deleteFileOrDirectoryWithRetry(File
file) {
}
public static void deleteDirectoryAndEmptyParent(File folder) {
-deleteFileOrDirectory(folder);
+deleteDirectoryAndEmptyParent(folder, null);
+ }
+
+ public static void deleteDirectoryAndEmptyParentWithRateLimiter(
+ File folder, LongConsumer deleteRateLimiter) {
+deleteDirectoryAndEmptyParent(folder, deleteRateLimiter);
+ }
+
+ private static void deleteDirectoryAndEmptyParent(File folder, LongConsumer
deleteRateLimiter) {
+deleteFileOrDirectory(folder, false, deleteRateLimiter);
final File parentFolder = folder.getParentFile();
File[] files = parentFolder.listFiles();
if (parentFolder.isDirectory() && (files == null || files.length == 0)) {
Review Comment:
Fixed in e84fe31364. `deleteDirectoryAndEmptyParent(...)` now treats
`parentFolder == null` as no parent to clean up, and I added
`testDeleteDirectoryAndEmptyParentWithRateLimiterAndNoParent` to cover the
relative-path/no-parent case.
##
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/utils/FileUtils.java:
##
@@ -111,13 +112,30 @@ public static void deleteFileOrDirectory(File file) {
}
public static void deleteFileOrDirectory(File file, boolean
quietForNoSuchFile) {
+deleteFileOrDirectory(file, quietForNoSuchFile, null);
+ }
+
+ public static void deleteFileOrDirectoryWithRateLimiter(
+ File file, LongConsumer deleteRateLimiter) {
+deleteFileOrDirectory(file, false, deleteRateLimiter);
+ }
+
+ public static void deleteFileOrDirectoryWithRateLimiter(
+ File file, boolean quietForNoSuchFile, LongConsumer deleteRateLimiter) {
+deleteFileOrDirectory(file, quietForNoSuchFile, deleteRateLimiter);
+ }
+
+ private static void deleteFileOrDirectory(
+ File file, boolean quietForNoSuchFile, LongConsumer deleteRateLimiter) {
if (file.isDirectory()) {
File[] files = file.listFiles();
if (files != null) {
for (File subfile : files) {
- deleteFileOrDirectory(subfile, quietForNoSuchFile);
+ deleteFileOrDirectory(subfile, quietForNoSuchFile,
deleteRateLimiter);
}
}
+} else if (deleteRateLimiter != null && file.isFile()) {
+ deleteRateLimiter.accept(file.length());
}
Review Comment:
Agreed, using the real file size for delete throttling is too conservative
because removal is mostly metadata work. Fixed in e84fe31364 by moving cleanup
to an independent `region_migration_file_remove_speed_limit_bytes_per_second`
limiter and charging fixed estimated remove costs instead: 64 KiB per regular
file and 4 KiB per directory. I also removed the IoTConsensusV2 transfer
limiter so regular consensus pipe replication is not throttled by the migration
transfer limit.
--
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]
Re: [PR] Extend region migration speed limit to transfers and deletion [iotdb]
jt2594838 commented on code in PR #18118:
URL: https://github.com/apache/iotdb/pull/18118#discussion_r3542760333
##
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/utils/FileUtils.java:
##
@@ -111,13 +112,30 @@ public static void deleteFileOrDirectory(File file) {
}
public static void deleteFileOrDirectory(File file, boolean
quietForNoSuchFile) {
+deleteFileOrDirectory(file, quietForNoSuchFile, null);
+ }
+
+ public static void deleteFileOrDirectoryWithRateLimiter(
+ File file, LongConsumer deleteRateLimiter) {
+deleteFileOrDirectory(file, false, deleteRateLimiter);
+ }
+
+ public static void deleteFileOrDirectoryWithRateLimiter(
+ File file, boolean quietForNoSuchFile, LongConsumer deleteRateLimiter) {
+deleteFileOrDirectory(file, quietForNoSuchFile, deleteRateLimiter);
+ }
+
+ private static void deleteFileOrDirectory(
+ File file, boolean quietForNoSuchFile, LongConsumer deleteRateLimiter) {
if (file.isDirectory()) {
File[] files = file.listFiles();
if (files != null) {
for (File subfile : files) {
- deleteFileOrDirectory(subfile, quietForNoSuchFile);
+ deleteFileOrDirectory(subfile, quietForNoSuchFile,
deleteRateLimiter);
}
}
+} else if (deleteRateLimiter != null && file.isFile()) {
+ deleteRateLimiter.accept(file.length());
}
Review Comment:
Deleting a file only involves some file system metadata modification and
inode reclaim, which will not introduce such io.
The rate-limiting step may slow down the migration significantly.
--
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]
Re: [PR] Extend region migration speed limit to transfers and deletion [iotdb]
Copilot commented on code in PR #18118:
URL: https://github.com/apache/iotdb/pull/18118#discussion_r3541235823
##
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/utils/FileUtils.java:
##
@@ -161,7 +179,16 @@ public static void deleteFileOrDirectoryWithRetry(File
file) {
}
public static void deleteDirectoryAndEmptyParent(File folder) {
-deleteFileOrDirectory(folder);
+deleteDirectoryAndEmptyParent(folder, null);
+ }
+
+ public static void deleteDirectoryAndEmptyParentWithRateLimiter(
+ File folder, LongConsumer deleteRateLimiter) {
+deleteDirectoryAndEmptyParent(folder, deleteRateLimiter);
+ }
+
+ private static void deleteDirectoryAndEmptyParent(File folder, LongConsumer
deleteRateLimiter) {
+deleteFileOrDirectory(folder, false, deleteRateLimiter);
final File parentFolder = folder.getParentFile();
File[] files = parentFolder.listFiles();
if (parentFolder.isDirectory() && (files == null || files.length == 0)) {
Review Comment:
`deleteDirectoryAndEmptyParent(...)` assumes `folder.getParentFile()` is
non-null. If the caller passes a path with no parent component (e.g., `new
File("dir")` or a root path on some platforms), this will throw a
`NullPointerException` at `parentFolder.listFiles()`, aborting the delete flow.
Please guard against `parentFolder == null` (or treat it as "no parent to clean
up").
--
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]
Re: [PR] Extend region migration speed limit to transfers and deletion [iotdb]
sonarqubecloud[bot] commented on PR #18118: URL: https://github.com/apache/iotdb/pull/18118#issuecomment-4890977288 ## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18118) **Quality Gate passed** Issues  [8 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=18118&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=18118&issueStatuses=ACCEPTED) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=18118&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=18118&metric=new_coverage&view=list)  [2.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=18118&metric=new_duplicated_lines_density&view=list) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=18118) -- 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]
