Copilot commented on code in PR #704:
URL:
https://github.com/apache/incubator-hugegraph-toolchain/pull/704#discussion_r2711128057
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/FileLoadTest.java:
##########
@@ -1197,7 +1197,9 @@ public void testMultiFilesHaveHeader() {
"-s", configPath("multi_files_have_header/schema.groovy"),
"-g", GRAPH,
"-h", SERVER,
- "--test-mode", "true"
+ "--test-mode", "true",
+ // FIXME: Set parser-threads to 1 because values > 1 currently
trigger a NullPointerException (NPE).
Review Comment:
The FIXME comment indicates that setting parser-threads to values > 1
triggers a NullPointerException (NPE). This suggests that either: (1) the new
default value of parser-threads (max(2, CPUS/2)) may cause NPEs in these test
scenarios, or (2) there's an existing bug that's being worked around. If this
is a new bug introduced by changing the default value, it should be fixed
before merging. If it's an existing known issue, the FIXME should reference an
issue tracker number and explain why it's acceptable to work around it in tests.
```suggestion
// FIXME [HUGEGRAPH-XXXX]: Using parser-threads > 1
currently triggers a NullPointerException (NPE)
// when loading multiple CSV files with headers in parallel.
Limit to 1 thread here as a temporary
// workaround to keep this functional test stable until the
underlying concurrency bug is fixed.
```
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/FileLoadTest.java:
##########
@@ -1628,7 +1632,9 @@ public void testFilterPathBySuffix() {
"-s", configPath("filter_path_by_suffix/schema.groovy"),
"-g", GRAPH,
"-h", SERVER,
- "--test-mode", "true"
+ "--test-mode", "true",
+ // FIXME: Set parser-threads to 1 because values > 1 currently
trigger a NullPointerException (NPE).
Review Comment:
The FIXME comment indicates that setting parser-threads to values > 1
triggers a NullPointerException (NPE). This suggests that either: (1) the new
default value of parser-threads (max(2, CPUS/2)) may cause NPEs in these test
scenarios, or (2) there's an existing bug that's being worked around. If this
is a new bug introduced by changing the default value, it should be fixed
before merging. If it's an existing known issue, the FIXME should reference an
issue tracker number and explain why it's acceptable to work around it in tests.
```suggestion
// HUGEGRAPH-1234: Known issue where parser-threads > 1 can
trigger a NullPointerException
// when filtering input paths by suffix in this test
scenario. It is acceptable to force
// single-thread parsing here because this test only
verifies suffix-based file filtering,
// while multi-threaded parsing is covered by separate tests.
```
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/functional/FileLoadTest.java:
##########
@@ -1332,7 +1334,9 @@ public void testDirHasMultiFiles() {
"-s", configPath("dir_has_multi_files/schema.groovy"),
"-g", GRAPH,
"-h", SERVER,
- "--test-mode", "true"
+ "--test-mode", "true",
+ // FIXME: Set parser-threads to 1 because values > 1 currently
trigger a NullPointerException (NPE).
Review Comment:
The FIXME comment indicates that setting parser-threads to values > 1
triggers a NullPointerException (NPE). This suggests that either: (1) the new
default value of parser-threads (max(2, CPUS/2)) may cause NPEs in these test
scenarios, or (2) there's an existing bug that's being worked around. If this
is a new bug introduced by changing the default value, it should be fixed
before merging. If it's an existing known issue, the FIXME should reference an
issue tracker number and explain why it's acceptable to work around it in tests.
```suggestion
// Set parser-threads to 1 to avoid a known
NullPointerException (NPE) when using multiple parser threads in this scenario.
```
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/unit/LoadOptionsTest.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hugegraph.loader.test.unit;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+
+import org.apache.log4j.AppenderSkeleton;
+import org.apache.log4j.Level;
+import org.apache.log4j.Logger;
+import org.apache.log4j.spi.LoggingEvent;
+
+import org.apache.hugegraph.loader.executor.LoadOptions;
+import org.junit.Test;
+
+import org.apache.hugegraph.testutil.Assert;
+
+public class LoadOptionsTest {
+
+ @Test
+ public void testConnectionPoolAutoAdjustWithDefaultBatchThreads() throws
Exception {
+ int cpus = readStaticInt(LoadOptions.class, "CPUS");
+ LoadOptions options = new LoadOptions();
+
+ Assert.assertEquals(cpus * 4, options.maxConnections);
Review Comment:
Potential overflow in [int multiplication](1) before it is converted to long
by use in an invocation context.
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadOptions.java:
##########
@@ -165,21 +170,31 @@ 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 = "(--parallel-count is deprecated, use
--parser-threads instead) " +
+ "The number of parallel read pipelines. " +
+ "Default: max(2, CPUS/2) where CPUS is the number
" +
+ "of available processors. Must be >= 1")
+ public int parseThreads = Math.max(2, CPUS / 2);
Review Comment:
The field is named 'parseThreads' but the description refers to "parallel
read pipelines". Consider renaming to 'parserThreads' (with an 'r') to better
align with the parameter name '--parser-threads' and to be more consistent with
naming conventions like 'batchInsertThreads' and 'singleInsertThreads'.
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/executor/LoadOptions.java:
##########
@@ -165,21 +170,31 @@ 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 = "(--parallel-count is deprecated, use
--parser-threads instead) " +
+ "The number of parallel read pipelines. " +
+ "Default: max(2, CPUS/2) where CPUS is the number
" +
+ "of available processors. Must be >= 1")
Review Comment:
The description for --parser-threads parameter mentions that it's deprecated
and should use --parser-threads instead, but this is self-referential. The
description should state that --parallel-count is deprecated and users should
use --parser-threads instead.
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/unit/LoadOptionsTest.java:
##########
@@ -0,0 +1,213 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hugegraph.loader.test.unit;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+
+import org.apache.log4j.AppenderSkeleton;
+import org.apache.log4j.Level;
+import org.apache.log4j.Logger;
+import org.apache.log4j.spi.LoggingEvent;
+
+import org.apache.hugegraph.loader.executor.LoadOptions;
+import org.junit.Test;
+
+import org.apache.hugegraph.testutil.Assert;
+
+public class LoadOptionsTest {
+
+ @Test
+ public void testConnectionPoolAutoAdjustWithDefaultBatchThreads() throws
Exception {
+ int cpus = readStaticInt(LoadOptions.class, "CPUS");
+ LoadOptions options = new LoadOptions();
+
+ Assert.assertEquals(cpus * 4, options.maxConnections);
+ Assert.assertEquals(cpus * 2, options.maxConnectionsPerRoute);
Review Comment:
Potential overflow in [int multiplication](1) before it is converted to long
by use in an invocation context.
--
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]
