[Feature] Decouple SeaTunnel Client and Server SEATUNNEL_HOME#10431
[Feature] Decouple SeaTunnel Client and Server SEATUNNEL_HOME#10431LiJie20190102 wants to merge 6 commits intoapache:devfrom
Conversation
|
Thanks for the contribution! This feature is very valuable for flexible deployment scenarios where client and server paths differ. However, after a detailed review, I found a critical bug in the ClassLoader cache management that will cause memory leaks, and a robustness issue in the path resolver. 1. Critical: Key Mismatch in ClassLoader Cache (Memory Leak)There is an inconsistency in how the cache
// DefaultClassLoaderService.java
// In getClassLoader:
String key = covertJarsToKey(jars); // Key is based on "file:$SEATUNNEL_HOME/..."
// In releaseClassLoader:
Collection<URL> resolvedJars = new ArrayList<>(jars);
PathResolver.resolvePathEnv(resolvedJars);
// ...
String key = covertJarsToKey(resolvedJars); // Key is based on "file:/opt/seatunnel/..."Consequence: The keys will never match. Fix Suggestion: In 2. NPE Risk in
|
Issue 1: Inconsistent URL Construction Leading to Path Format MismatchLocation: Problem Description:
Potential Risks:
Impact Scope:
Severity: MAJOR Improvement Suggestions: // In replacePathWithEnv, keep URL format consistent
public static URL replacePathWithEnv(URL url) {
// ... existing logic ...
if (normalizedPath.startsWith(normalizedHome)) {
String relativePath = normalizedPath.substring(normalizedHome.length());
// Consistent with resolvePathEnv: use URI constructor
String newPath = SEATUNNEL_HOME_VAR + "/" + relativePath.replace(File.separatorChar, '/');
try {
// Use URI constructor to build URL, ensure format consistency
return new java.net.URI(url.getProtocol(), url.getHost(), newPath, null).toURL();
} catch (MalformedURLException | URISyntaxException e) {
throw new RuntimeException("Failed to create logical URL for: " + url, e);
}
}
return url;
}Rationale: Uniformly use URI to construct URLs, ensuring URL formats generated by Issue 2: Redundant Path Prefix Handling Logic in resolvePathEnvLocation: Problem Description:
Potential Risks:
Impact Scope:
Severity: MINOR Improvement Suggestions: public static URL resolvePathEnv(URL url) {
String path = url.getPath();
if (!path.contains(SEATUNNEL_HOME_VAR)) {
return url;
}
String home = Common.getSeaTunnelHome();
if (StringUtils.isBlank(home)) {
return url;
}
// Handle path prefix uniformly
// Normalize path: remove leading slashes, replace variables
String cleanPath = path.startsWith("/") ? path.substring(1) : path;
String relativePath = cleanPath.replace(SEATUNNEL_HOME_VAR, "");
relativePath = relativePath.replaceAll("^/+", ""); // Remove all leading slashes
Path fullPath = Paths.get(home, relativePath);
try {
return fullPath.toUri().toURL();
} catch (MalformedURLException e) {
throw new RuntimeException("Failed to resolve logical URL for: " + url, e);
}
}Rationale: Simplify logic, unify path prefix handling, improve code readability and maintainability. Issue 3: PathResolver's Collection Modification Methods May Cause Concurrency IssuesLocation: Problem Description: urls.clear();
urls.addAll(replaced);Potential Risks:
Impact Scope:
Severity: MAJOR Improvement Suggestions: /**
* Replaces the absolute path of SEATUNNEL_HOME in the given URLs with a logical variable.
* Note: This method modifies the collection in-place. If you need to preserve the original
* collection, create a copy before calling this method.
*
* @param urls The collection of absolute URLs (will be modified)
*/
public static void replacePathWithEnv(Collection<URL> urls) {
if (urls == null || urls.isEmpty()) {
return;
}
List<URL> replaced = urls.stream().map(PathResolver::replacePathWithEnv).collect(Collectors.toList());
urls.clear();
urls.addAll(replaced);
}
// Or, provide a version that does not modify the collection
public static Collection<URL> replacePathWithEnvCopy(Collection<URL> urls) {
if (urls == null || urls.isEmpty()) {
return urls;
}
return urls.stream()
.map(PathResolver::replacePathWithEnv)
.collect(Collectors.toList());
}Rationale:
Issue 4: DefaultClassLoaderService's Error Message Is MisleadingLocation: Problem Description: But in the new design, client and server paths can be different (decoupled through SEATUNNEL_HOME). Potential Risks:
Impact Scope:
Severity: MINOR Improvement Suggestions: if (!file.exists()) {
String host = ((NodeEngineImpl) nodeEngine).getNode().getThisAddress().getHost();
String jarPath = jar.toString();
String errorMsg = String.format(
"The jar file %s cannot be found on node %s. " +
"Current SEATUNNEL_HOME: %s. " +
"Please ensure: " +
"1) SEATUNNEL_HOME is correctly configured on this node, " +
"2) The connector JARs are installed in $SEATUNNEL_HOME/connectors or $SEATUNNEL_HOME/plugins, " +
"3) Run 'bin/install-plugin.sh' to install connectors if needed.",
jarPath, host, Common.getSeaTunnelHome());
throw new ClassLoaderException(ClassLoaderErrorCode.NOT_FOUND_JAR, errorMsg);
}Rationale: Provide more accurate and helpful error messages to help users quickly locate problems. Issue 5: PathResolver's Exception Handling Is Too BroadLocation: Problem Description: Potential Risks:
Impact Scope:
Severity: MINOR Improvement Suggestions: public static URL replacePathWithEnv(URL url) {
// ... existing logic ...
try {
return new URL(url.getProtocol(), url.getHost(), url.getPort(), newPath);
} catch (MalformedURLException e) {
throw new RuntimeException(
String.format(
"Failed to create logical URL. Original URL: %s, New path: %s, " +
"SEATUNNEL_HOME: %s. Please check if the URL format is valid.",
url, newPath, Common.getSeaTunnelHome()),
e);
}
}
public static URL resolvePathEnv(URL url) {
// ... existing logic ...
try {
return fullPath.toUri().toURL();
} catch (MalformedURLException e) {
throw new RuntimeException(
String.format(
"Failed to resolve logical URL. Logical URL: %s, Resolved path: %s, " +
"SEATUNNEL_HOME: %s. Please check if SEATUNNEL_HOME is correctly configured.",
url, fullPath, Common.getSeaTunnelHome()),
e);
}
}Rationale: Provide more detailed error information, including original URL, processed path, SEATUNNEL_HOME value, etc., to help diagnose problems. Issue 6: Missing Handling for SEATUNNEL_HOME Being Empty or Containing Special CharactersLocation: Problem Description:
Potential Risks:
Impact Scope:
Severity: MINOR Improvement Suggestions: public static URL resolvePathEnv(URL url) {
String path = url.getPath();
if (!path.contains(SEATUNNEL_HOME_VAR)) {
return url;
}
String home = Common.getSeaTunnelHome();
if (StringUtils.isBlank(home)) {
return url;
}
// Normalize home path
try {
Path normalizedHome = Paths.get(home).normalize();
if (!Files.exists(normalizedHome)) {
throw new RuntimeException(
String.format("SEATUNNEL_HOME does not exist: %s", home));
}
home = normalizedHome.toAbsolutePath().toString();
} catch (InvalidPathException e) {
throw new RuntimeException(
String.format("Invalid SEATUNNEL_HOME: %s. Error: %s", home, e.getMessage()), e);
}
// ... continue processing ...
}Rationale: Validate and normalize SEATUNNEL_HOME before processing paths, detecting configuration issues early. Issue 7: Incomplete Test CoverageLocation: Problem Description:
Potential Risks:
Impact Scope:
Severity: MINOR Improvement Suggestions: @Test
public void testJarInRootOfSeaTunnelHome() throws MalformedURLException {
String home = "/opt/seatunnel";
System.setProperty("SEATUNNEL_HOME", home);
Common.setSeaTunnelHome(home);
String jarPath = home + "/plugin.jar";
URL absoluteUrl = new File(jarPath).toURI().toURL();
URL logicalUrl = PathResolver.replacePathWithEnv(absoluteUrl);
Assertions.assertEquals("$SEATUNNEL_HOME/plugin.jar", logicalUrl.getPath());
}
@Test
public void testSeaTunnelHomeWithSpaces() throws MalformedURLException {
String home = "/opt/sea tunnel/home";
System.setProperty("SEATUNNEL_HOME", home);
Common.setSeaTunnelHome(home);
String jarPath = home + "/lib/test.jar";
URL absoluteUrl = new File(jarPath).toURI().toURL();
URL logicalUrl = PathResolver.replacePathWithEnv(absoluteUrl);
Assertions.assertTrue(logicalUrl.getPath().contains("$SEATUNNEL_HOME"));
}
@Test
public void testEmptyRelativePath() throws MalformedURLException {
// Test when path exactly equals SEATUNNEL_HOME
String home = "/opt/seatunnel";
System.setProperty("SEATUNNEL_HOME", home);
Common.setSeaTunnelHome(home);
// This case is not very practical, but test boundary conditions
URL absoluteUrl = new File(home).toURI().toURL();
URL logicalUrl = PathResolver.replacePathWithEnv(absoluteUrl);
// Should return the original URL, because JAR file should not be a directory
Assertions.assertEquals(absoluteUrl, logicalUrl);
}Rationale: Improve test coverage to ensure code works correctly under various edge conditions. Issue 8: Missing DocumentationLocation: PR overall Problem Description:
Potential Risks:
Impact Scope:
Severity: MINOR Improvement Suggestions: /**
* Utility class for resolving SeaTunnel paths between client and server.
*
* <p>In a distributed deployment where the client and server have different
* SEATUNNEL_HOME paths, JAR file paths must be translated between the two environments.
* This class provides methods to:
* <ul>
* <li>Replace absolute paths with logical variables (on the client side)</li>
* <li>Resolve logical variables back to absolute paths (on the server side)</li>
* </ul>
*
* <h3>Usage Example</h3>
* <pre>
* // Client side (before serialization)
* URL clientJar = new URL("file:/opt/client/lib/connector.jar");
* URL logicalJar = PathResolver.replacePathWithEnv(clientJar);
* // logicalJar is now: file:$SEATUNNEL_HOME/lib/connector.jar
*
* // Server side (after deserialization)
* URL serverJar = PathResolver.resolvePathEnv(logicalJar);
* // serverJar is now: file:/opt/server/lib/connector.jar
* </pre>
*
* @since 2.3.5
*/
public class PathResolver {
// ...
}Rationale: Provide clear class-level documentation explaining purpose, usage scenarios, and examples. |
...re/src/main/java/org/apache/seatunnel/engine/core/classloader/DefaultClassLoaderService.java
Outdated
Show resolved
Hide resolved
acbed22 to
459f183
Compare



Purpose of this pull request
Implement feature #10167
Does this PR introduce any user-facing change?
How was this patch tested?
1、Deploy the master node and the slave node on the same machine, set ST_HOME to
/opt/soft/st0201/server, and configure the network settings inhazelcast-master.yamlandhazelcast-worker.yaml2、Deploy the client on another server, with ST_HOME set to
/opt/soft/st0201/client, and configure the network settings inhazelcast-client.yaml. This ensures that the package paths for the server and client are different.3、Submit the task on the client side, such as
bin/seatunnel.sh --config config/v2.batch.config.template. After submitting the task, check whether it is executed successfully, and ensure that the server logs do not contain any client file resourcesSelf-test results
Before
1 client log:
Check list
New License Guide
incompatible-changes.mdto describe the incompatibility caused by this PR.