Copilot commented on code in PR #3483:
URL: https://github.com/apache/parquet-java/pull/3483#discussion_r3193224268
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/InterOpTester.java:
##########
@@ -34,7 +36,30 @@ public class InterOpTester {
private static final String PARQUET_TESTING_REPO =
"https://github.com/apache/parquet-testing/raw/";;
private static final String PARQUET_TESTING_PATH = "target/parquet-testing/";
private static final Logger LOG =
LoggerFactory.getLogger(InterOpTester.class);
- private OkHttpClient httpClient = new OkHttpClient();
+ // since PARQUET_TESTING_REPO might be beyond a web proxy ...
+ public static OkHttpClient createOkHttpClientOptProxy() {
+ /* We use a different JVM property set,
+ * because CI may define JVM properties
+ * "https.proxyHost" and "https.proxyPort"
+ * and that proxy won't support some compressions
+ * (e.g. gzip/snappy on github.com CI).
+ */
+ String proxyHost = System.getProperty("parquet.https.proxyHost");
+ String proxyPort = System.getProperty("parquet.https.proxyPort");
+ OkHttpClient client = null;
+ if (proxyHost != null || proxyPort != null) {
+ try {
+ int port = Integer.valueOf(proxyPort);
+ Proxy proxy = new Proxy(Proxy.Type.HTTP, new
InetSocketAddress(proxyHost, port));
+ client = new OkHttpClient.Builder().proxy(proxy).build();
+ } catch (NumberFormatException e) {
+ }
Review Comment:
`proxyHost`/`proxyPort` validation is unsafe here: the `if (proxyHost !=
null || proxyPort != null)` condition allows one to be null, but
`Integer.valueOf(proxyPort)` will throw a `NullPointerException` when only
`proxyHost` is set (and `InetSocketAddress(proxyHost, port)` will also fail if
`proxyHost` is null). Require both properties to be non-null/non-empty before
building the proxy, and handle invalid/missing values explicitly (ideally with
a log message) so tests don’t fail unexpectedly.
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/InterOpTester.java:
##########
@@ -34,7 +36,30 @@ public class InterOpTester {
private static final String PARQUET_TESTING_REPO =
"https://github.com/apache/parquet-testing/raw/";;
private static final String PARQUET_TESTING_PATH = "target/parquet-testing/";
private static final Logger LOG =
LoggerFactory.getLogger(InterOpTester.class);
- private OkHttpClient httpClient = new OkHttpClient();
+ // since PARQUET_TESTING_REPO might be beyond a web proxy ...
+ public static OkHttpClient createOkHttpClientOptProxy() {
+ /* We use a different JVM property set,
+ * because CI may define JVM properties
+ * "https.proxyHost" and "https.proxyPort"
+ * and that proxy won't support some compressions
+ * (e.g. gzip/snappy on github.com CI).
+ */
+ String proxyHost = System.getProperty("parquet.https.proxyHost");
+ String proxyPort = System.getProperty("parquet.https.proxyPort");
+ OkHttpClient client = null;
Review Comment:
The PR description says it reads standard JVM properties
`https.proxyHost`/`https.proxyPort`, but the implementation reads
`parquet.https.proxyHost`/`parquet.https.proxyPort`. Please align the
implementation and description (either switch to the standard properties, or
document that users must set the `parquet.*` properties and explain why).
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/InterOpTester.java:
##########
@@ -34,7 +36,30 @@ public class InterOpTester {
private static final String PARQUET_TESTING_REPO =
"https://github.com/apache/parquet-testing/raw/";;
private static final String PARQUET_TESTING_PATH = "target/parquet-testing/";
private static final Logger LOG =
LoggerFactory.getLogger(InterOpTester.class);
- private OkHttpClient httpClient = new OkHttpClient();
+ // since PARQUET_TESTING_REPO might be beyond a web proxy ...
+ public static OkHttpClient createOkHttpClientOptProxy() {
+ /* We use a different JVM property set,
+ * because CI may define JVM properties
+ * "https.proxyHost" and "https.proxyPort"
+ * and that proxy won't support some compressions
+ * (e.g. gzip/snappy on github.com CI).
+ */
+ String proxyHost = System.getProperty("parquet.https.proxyHost");
+ String proxyPort = System.getProperty("parquet.https.proxyPort");
+ OkHttpClient client = null;
+ if (proxyHost != null || proxyPort != null) {
+ try {
+ int port = Integer.valueOf(proxyPort);
+ Proxy proxy = new Proxy(Proxy.Type.HTTP, new
InetSocketAddress(proxyHost, port));
+ client = new OkHttpClient.Builder().proxy(proxy).build();
+ } catch (NumberFormatException e) {
Review Comment:
The `catch (NumberFormatException e) { }` block is empty, which makes proxy
misconfiguration very hard to diagnose (the code silently falls back to a
direct connection). Consider logging a warning that includes the property
names/values when parsing fails so developers can understand why the proxy
wasn’t applied.
--
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]
