Copilot commented on code in PR #704:
URL:
https://github.com/apache/incubator-hugegraph-toolchain/pull/704#discussion_r2702529936
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadOptions.java:
##########
@@ -165,21 +169,30 @@ public final class LoadOptions implements Cloneable {
public int singleInsertThreads = 8;
@Parameter(names = {"--max-conn"}, arity = 1,
- description = "Max number of HTTP connections to server")
- public int maxConnections = CPUS * 4;
+ validateWith = {PositiveValidator.class},
+ description = "Max number of HTTP connections to server. " +
+ "If left as default and batch-insert-threads is "
+
+ "set, this may be auto-adjusted")
+ public int maxConnections = DEFAULT_MAX_CONNECTIONS;
@Parameter(names = {"--max-conn-per-route"}, arity = 1,
- description = "Max number of HTTP connections to each route")
- public int maxConnectionsPerRoute = CPUS * 2;
+ validateWith = {PositiveValidator.class},
+ description = "Max number of HTTP connections to each route. " +
+ "If left as default and batch-insert-threads is "
+
+ "set, this may be auto-adjusted")
+ public int maxConnectionsPerRoute = DEFAULT_MAX_CONNECTIONS_PER_ROUTE;
@Parameter(names = {"--batch-size"}, arity = 1,
validateWith = {PositiveValidator.class},
description = "The number of lines in each submit")
public int batchSize = 500;
- @Parameter(names = {"--parallel-count"}, arity = 1,
- description = "The number of parallel read pipelines")
- public int parallelCount = 1;
+ @Parameter(names = {"--parallel-count", "--parser-threads"}, arity = 1,
+ validateWith = {PositiveValidator.class},
+ description = "The number of parallel read pipelines. " +
+ "Default: auto max(2, cpu/2). " +
+ "Must be >= 1")
Review Comment:
The description text is unclear. "Default: auto max(2, cpu/2)" is confusing
- consider rephrasing to: "Default: max(2, CPUS/2) where CPUS is the number of
available processors"
```suggestion
"Default: max(2, CPUS/2) where CPUS is the
number " +
"of available processors. Must be >= 1")
```
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/task/TaskManager.java:
##########
@@ -152,13 +156,12 @@ public void submitBatch(InputStruct struct,
ElementMapping mapping,
CompletableFuture.runAsync(task, this.batchService).whenComplete(
(r, e) -> {
if (e != null) {
- LOG.warn("Batch insert {} error, try single insert",
- mapping.type(), e);
- // The time of single insert is counted separately
- this.submitInSingle(struct, mapping, batch);
- } else {
- summary.metrics(struct).minusFlighting(batch.size());
+ LOG.error("Batch insert {} error, interrupting import",
mapping.type(), e);
+ this.context.stopLoading();
+ Printer.printError("Batch insert %s failed, stop loading,
Please check the logs",
Review Comment:
Inconsistent capitalization in error message. The sentence should either
capitalize "Please" as it starts a new sentence, or use a comma with lowercase
"please" to continue the sentence. Consider changing to: "Batch insert %s
failed, stop loading. Please check the logs"
```suggestion
Printer.printError("Batch insert %s failed, stop
loading. Please check the logs",
```
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/task/TaskManager.java:
##########
@@ -152,13 +156,12 @@ public void submitBatch(InputStruct struct,
ElementMapping mapping,
CompletableFuture.runAsync(task, this.batchService).whenComplete(
(r, e) -> {
if (e != null) {
- LOG.warn("Batch insert {} error, try single insert",
- mapping.type(), e);
- // The time of single insert is counted separately
- this.submitInSingle(struct, mapping, batch);
- } else {
- summary.metrics(struct).minusFlighting(batch.size());
+ LOG.error("Batch insert {} error, interrupting import",
mapping.type(), e);
+ this.context.stopLoading();
+ Printer.printError("Batch insert %s failed, stop loading,
Please check the logs",
+ mapping.type().string());
}
Review Comment:
The new batch insert error handling behavior (stopping all loading on batch
insert failure instead of falling back to single insert) is a significant
change that lacks test coverage. Consider adding a test that verifies the
loading process stops correctly when a batch insert fails, and that the
appropriate error message is displayed.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
