Skip to content

Conversation

@snuyanzin
Copy link
Contributor

What is the purpose of the change

This PR resolves libraries extraction issue from jars

Brief change log

  • Always in case of zip/jar use '/' path separator
  • Test with generated jar emulating the real case

Verifying this change

  • Added test generates fake jar with a structure
    test.jar
    |- lib
    |--|- internalTest.jar
    and then calls for PackagedProgram#extractContainedLibraries to check if it extracts internalTest.jar correctly

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@Test
public void testExtractContainedLibraries() {
String s = "testExtractContainedLibraries";
Path workDir = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use junit TemporaryFolder instead

Path workDir = null;
Path fakeJar = null;
try {
workDir = Files.createTempDirectory(PackagedProgram.class.getSimpleName() + "_");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derive from TemporaryFolder

}
PackagedProgram.extractContainedLibraries(fakeJar.toUri().toURL());
}
catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to catch them, just let them bubble up

System.err.println(e.getMessage());
e.printStackTrace();
Assert.fail("Test is erroneous: " + e.getMessage());
} finally {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this block will be unnecessary with the usage of TemporaryFolder

zos.putNextEntry(entry);
zos.write(s.getBytes());
zos.closeEntry();
} catch (IOException e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, just let hte exception bubble up.

workDir = Files.createTempDirectory(PackagedProgram.class.getSimpleName() + "_");
fakeJar = Paths.get(workDir.toAbsolutePath().toString(), "test.jar");
FileOutputStream fos = new FileOutputStream(fakeJar.toFile());
try (ZipOutputStream zos = new ZipOutputStream(fos)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline FileOutputStream creation, i.e. new ZipOutputStream(new FileOutputStream(...))

PackagedProgramTest#testExtractContainedLibraries to check PackagedProgram#extractContainedLibraries
Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, will try it out locally.

public class PackagedProgramTest {

@Rule
public TemporaryFolder temporaryFolder = new TemporaryFolder();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be final

@zentol
Copy link
Contributor

zentol commented Jul 10, 2018

If it work I'll fix the issue I found and merge it afterwards.

@snuyanzin
Copy link
Contributor Author

ok, thank you for review
about public TemporaryFolder temporaryFolder = new TemporaryFolder(); could be final - agree.
However while searching for usages I realized that in most cases it is not final but not changed.
Am I right that in spite of existing code at least for newly created test classes it is better to use temporaryFolder as final?

@zentol
Copy link
Contributor

zentol commented Jul 10, 2018

yes, TemporaryFolders should always be final, regardless of what existing code does.

zentol pushed a commit to zentol/flink that referenced this pull request Jul 10, 2018
zentol pushed a commit to zentol/flink that referenced this pull request Jul 10, 2018
zentol pushed a commit to zentol/flink that referenced this pull request Jul 10, 2018
zentol pushed a commit to zentol/flink that referenced this pull request Jul 10, 2018
zentol pushed a commit to zentol/flink that referenced this pull request Jul 10, 2018
zentol pushed a commit to zentol/flink that referenced this pull request Jul 10, 2018
zentol pushed a commit to zentol/flink that referenced this pull request Jul 11, 2018
zentol pushed a commit to zentol/flink that referenced this pull request Jul 11, 2018
@asfgit asfgit closed this in 2fbe562 Jul 11, 2018
sampathBhat pushed a commit to sampathBhat/flink that referenced this pull request Jul 26, 2018
@snuyanzin snuyanzin deleted the FLINK_9743 branch August 7, 2018 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants