MartijnVisser commented on code in PR #27026:
URL: https://github.com/apache/flink/pull/27026#discussion_r3117116323
##########
flink-filesystems/flink-s3-fs-hadoop/pom.xml:
##########
@@ -272,6 +295,12 @@ under the License.
<exclude>mozilla/**</exclude>
<exclude>META-INF/maven/**</exclude>
<exclude>META-INF/LICENSE.txt</exclude>
+
<!-- Exclude GPL-licensed content from Hadoop transitive dependencies -->
+
<exclude>com/sun/jersey/**</exclude>
+
<exclude>com/sun/xml/bind/**</exclude>
+
<exclude>THIRD-PARTY-NOTICES</exclude>
+
<exclude>LICENSE</exclude>
+
<exclude>NOTICE</exclude>
Review Comment:
Why are we excluding these?
##########
flink-clients/src/test/java/org/apache/flink/client/deployment/application/PackagedProgramApplicationITCase.java:
##########
@@ -71,6 +72,7 @@
import static org.junit.jupiter.api.Assertions.assertNull;
/** Integration tests related to {@link PackagedProgramApplication}. */
+@Isolated("MiniCluster tests with leadership changes that can interfere with
parallel tests")
Review Comment:
Why are we doing this for tests that aren't related to this upgrade?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestServerEndpointITCase.java:
##########
@@ -116,6 +117,7 @@
/** IT cases for {@link RestClient} and {@link RestServerEndpoint}. */
@ExtendWith(ParameterizedTestExtension.class)
+@Isolated("Network timing-sensitive tests that can fail with parallel test
execution")
Review Comment:
Same here. If all of these tests are unstable, we shouldn't make the changes
as part of this work as it makes tracking the work tricky, but file separate
test stability Jira's for them
##########
flink-runtime/src/test/java/org/apache/flink/runtime/executiongraph/ExecutionVertexCancelTest.java:
##########
@@ -48,6 +49,7 @@
import static org.assertj.core.api.Assertions.fail;
/** Tests for cancelling {@link ExecutionVertex ExecutionVertices}. */
+@Isolated("Uses shared executor that can cause interference with parallel
tests")
Review Comment:
Same here, why are we doing this for an unrelated test?
##########
flink-filesystems/flink-s3-fs-presto/pom.xml:
##########
@@ -562,6 +562,12 @@ under the License.
<exclude>mime.types</exclude>
<exclude>mozilla/**</exclude>
<exclude>META-INF/LICENSE.txt</exclude>
+
<!-- Exclude GPL-licensed content from Hadoop transitive dependencies -->
+
<exclude>com/sun/jersey/**</exclude>
+
<exclude>com/sun/xml/bind/**</exclude>
+
<exclude>THIRD-PARTY-NOTICES</exclude>
+
<exclude>LICENSE</exclude>
+
<exclude>NOTICE</exclude>
Review Comment:
Why are we excluding these?
##########
flink-filesystems/flink-s3-fs-hadoop/pom.xml:
##########
@@ -272,6 +295,12 @@ under the License.
<exclude>mozilla/**</exclude>
<exclude>META-INF/maven/**</exclude>
<exclude>META-INF/LICENSE.txt</exclude>
+
<!-- Exclude GPL-licensed content from Hadoop transitive dependencies -->
+
<exclude>com/sun/jersey/**</exclude>
+
<exclude>com/sun/xml/bind/**</exclude>
Review Comment:
These aren't GPL licensed, right? Can we get a full print of which
Jersey/Bind dependencies would normally be pulled in, and then see if they're
indeed not needed. I'm afraid we'll run into issues because the implementation
might actually depend on them.
##########
flink-filesystems/flink-s3-fs-hadoop/src/test/java/org/apache/flink/fs/s3hadoop/S3ValidationApp.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.flink.fs.s3hadoop;
+
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.core.fs.FSDataInputStream;
+import org.apache.flink.core.fs.FSDataOutputStream;
+import org.apache.flink.core.fs.FileStatus;
+import org.apache.flink.core.fs.FileSystem;
+import org.apache.flink.core.fs.FileSystem.WriteMode;
+import org.apache.flink.core.fs.Path;
+
+import java.io.ByteArrayOutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.UUID;
+
+/**
+ * A standalone application to validate S3 filesystem integration with AWS SDK
v2.
+ *
+ * <p>Usage: S3ValidationApp bucket accessKey secretKey
+ *
+ * <p>This application performs basic S3 operations (write, read, list,
delete) to verify the Hadoop
+ * S3 filesystem is working correctly.
+ */
+public class S3ValidationApp {
Review Comment:
Is this class a duplicate of `S3PerformanceTest.testSmallFileLatency` ? Or
should it be an `ITCase` ?
##########
flink-filesystems/flink-s3-fs-presto/pom.xml:
##########
@@ -562,6 +562,12 @@ under the License.
<exclude>mime.types</exclude>
<exclude>mozilla/**</exclude>
<exclude>META-INF/LICENSE.txt</exclude>
+
<!-- Exclude GPL-licensed content from Hadoop transitive dependencies -->
Review Comment:
Does this mean that the Presto JAR now bundles AWS SDK v2 along SDK v1, even
if Presto doesn't use it? Should we exclude it?
##########
flink-filesystems/flink-s3-fs-base/pom.xml:
##########
@@ -32,6 +32,7 @@ under the License.
<properties>
<fs.s3.aws.version>1.12.779</fs.s3.aws.version>
+ <fs.s3.aws.sdk2.version>2.29.52</fs.s3.aws.sdk2.version>
Review Comment:
How have we picked this version? Could we add a comment on why we did that?
##########
flink-filesystems/flink-s3-fs-presto/pom.xml:
##########
@@ -562,6 +562,12 @@ under the License.
<exclude>mime.types</exclude>
<exclude>mozilla/**</exclude>
<exclude>META-INF/LICENSE.txt</exclude>
+
<!-- Exclude GPL-licensed content from Hadoop transitive dependencies -->
+
<exclude>com/sun/jersey/**</exclude>
+
<exclude>com/sun/xml/bind/**</exclude>
Review Comment:
These aren't GPL licensed, right? Can we get a full print of which
Jersey/Bind dependencies would normally be pulled in, and then see if they're
indeed not needed. I'm afraid we'll run into issues because the implementation
might actually depend on them.
--
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]
