You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by jaredjstewart <gi...@git.apache.org> on 2017/03/17 21:15:18 UTC

[GitHub] geode pull request #429: Geode-2686

GitHub user jaredjstewart opened a pull request:

    https://github.com/apache/geode/pull/429

    Geode-2686

    GEODE-2686: Remove JarClassLoader  \u2026
     - Remove JarClassLoader
     - Replace ClassPathLoader's collection of JarClassLoaders with a single URLClassLoader
     - Change naming scheme for deployed jars from 'vf.gf#myJar.jar#1' to 'myJar.v1.jar'
    
    Change singleton implementation of ClassPathLoader from AtomicReference to volatile
    
    JarDeployer is threadsafe

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jaredjstewart/geode GEODE-2686

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/geode/pull/429.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #429
    
----
commit 7788dba702a58e2c3997b974badd8e45e861b47f
Author: Jared Stewart <js...@pivotal.io>
Date:   2017-01-19T20:00:04Z

    GEODE-2686: Remove JarClassLoader
    
     - Remove JarClassLoader
     - Replace ClassPathLoader's collection of JarClassLoaders with a single URLClassLoader
     - Change naming scheme for deployed jars from 'vf.gf#myJar.jar#1' to 'myJar.v1.jar'

commit b9e511262b853e6f7ae346eb414138813c0e7a2d
Author: Jared Stewart <js...@pivotal.io>
Date:   2017-03-17T00:22:59Z

    Change singleton implementation of ClassPathLoader from AtomicReference to volatile

commit 5d50b12cc389293246545f08608663df9f962419
Author: Jared Stewart <js...@pivotal.io>
Date:   2017-03-17T20:59:32Z

    JarDeployer is threadsafe

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107045945
  
    --- Diff: geode-core/src/test/java/org/apache/geode/internal/ClassPathLoaderTest.java ---
    @@ -65,6 +65,26 @@ public void testLatestExists() throws Exception {
         assertNotNull(ClassPathLoader.getLatest());
       }
     
    +  @Test
    +  public void testZeroLengthFile() throws IOException, ClassNotFoundException {
    +    try {
    +      ClassPathLoader.getLatest().getJarDeployer().deploy(new String[] {"JarDeployerDUnitZLF.jar"},
    +          new byte[][] {new byte[0]});
    +      fail("Zero length files are not deployable");
    +    } catch (IllegalArgumentException expected) {
    --- End diff --
    
    Better to use AssertJ assertThatThrownBy or CatchException. This is ok though.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107048991
  
    --- Diff: geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java ---
    @@ -144,40 +131,99 @@ public void run() {
           Thread.sleep(500);
           alternateDir.mkdir();
           thread.join();
    -    } catch (Exception e) {
    -      fail(e);
    +    } catch (InterruptedException iex) {
    +      fail("Interrupted while waiting.");
    --- End diff --
    
    In particular, this is the worst thing we as developers can do in a JUnit Test. This eats the exception and then throws an entirely new Exception stack without the original stack trace. All JUnit @Test methods should simple have "throws Exception" and then avoid this stuff.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107192575
  
    --- Diff: geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java ---
    @@ -39,83 +44,63 @@
       @Rule
       public TemporaryFolder temporaryFolder = new TemporaryFolder();
     
    -  @Rule
    -  public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
    +  JarDeployer jarDeployer;
     
       @Before
       public void setup() {
    -    System.setProperty("user.dir", temporaryFolder.getRoot().getAbsolutePath());
         classBuilder = new ClassBuilder();
    -    ClassPathLoader.setLatestToDefault();
    +    jarDeployer = new JarDeployer(temporaryFolder.getRoot());
       }
     
    -  @Test
    -  public void testDeployFileAndChange() throws Exception {
    -    final JarDeployer jarDeployer = new JarDeployer();
    +  private byte[] createJarWithClass(String className) throws IOException {
    +    String stringBuilder = "package integration.parent;" + "public class " + className + " {}";
     
    -    // First deploy of the JAR file
    -    byte[] jarBytes = this.classBuilder.createJarFromName("ClassA");
    -    JarClassLoader jarClassLoader =
    -        jarDeployer.deploy(new String[] {"JarDeployerDUnit.jar"}, new byte[][] {jarBytes})[0];
    -    File deployedJar = new File(jarClassLoader.getFileCanonicalPath());
    +    return this.classBuilder.createJarFromClassContent("integration/parent/" + className,
    +        stringBuilder);
    +  }
     
    -    assertThat(deployedJar).exists();
    -    assertThat(deployedJar.getName()).contains("#1");
    -    assertThat(deployedJar.getName()).doesNotContain("#2");
    +  @Test
    +  public void testFileVersioning() throws IOException, ClassNotFoundException {
    +    String jarName = "JarDeployerIntegrationTest.jar";
     
    -    assertThat(ClassPathLoader.getLatest().forName("ClassA")).isNotNull();
    +    byte[] firstJarBytes = createJarWithClass("ClassA");
     
    -    assertThat(doesFileMatchBytes(deployedJar, jarBytes));
    +    // First deploy of the JAR file
    +    DeployedJar firstDeployedJar = jarDeployer.deployWithoutRegistering(jarName, firstJarBytes);
     
    -    // Now deploy an updated JAR file and make sure that the next version of the JAR file
    -    // was created and the first one was deleted.
    -    jarBytes = this.classBuilder.createJarFromName("ClassB");
    -    JarClassLoader newJarClassLoader =
    -        jarDeployer.deploy(new String[] {"JarDeployerDUnit.jar"}, new byte[][] {jarBytes})[0];
    -    File nextDeployedJar = new File(newJarClassLoader.getFileCanonicalPath());
    +    assertThat(firstDeployedJar.getFile()).exists().hasBinaryContent(firstJarBytes);
    +    assertThat(firstDeployedJar.getFile().getName()).contains(".v1.").doesNotContain(".v2.");
     
    -    assertThat(nextDeployedJar.exists());
    -    assertThat(nextDeployedJar.getName()).contains("#2");
    -    assertThat(doesFileMatchBytes(nextDeployedJar, jarBytes));
    +    // Now deploy an updated JAR file and make sure that the next version of the JAR file
    +    // was created
    +    byte[] secondJarBytes = createJarWithClass("ClassB");
     
    -    assertThat(ClassPathLoader.getLatest().forName("ClassB")).isNotNull();
    +    DeployedJar secondDeployedJar = jarDeployer.deployWithoutRegistering(jarName, secondJarBytes);
    +    File secondDeployedJarFile = new File(secondDeployedJar.getFileCanonicalPath());
     
    -    assertThatThrownBy(() -> ClassPathLoader.getLatest().forName("ClassA"))
    -        .isExactlyInstanceOf(ClassNotFoundException.class);
    +    assertThat(secondDeployedJarFile).exists().hasBinaryContent(secondJarBytes);
    +    assertThat(secondDeployedJarFile.getName()).contains(".v2.").doesNotContain(".v1.");
     
    -    assertThat(jarDeployer.findSortedOldVersionsOfJar("JarDeployerDUnit.jar")).hasSize(1);
    +    File[] sortedOldJars = jarDeployer.findSortedOldVersionsOfJar(jarName);
    +    assertThat(sortedOldJars).hasSize(2);
    +    assertThat(sortedOldJars[0].getName()).contains(".v2.");
    +    assertThat(sortedOldJars[1].getName()).contains(".v1.");
         assertThat(jarDeployer.findDistinctDeployedJars()).hasSize(1);
       }
     
       @Test
    -  public void testDeployNoUpdateWhenNoChange() throws Exception {
    -    final JarDeployer jarDeployer = new JarDeployer();
    -
    -    // First deploy of the JAR file
    -    byte[] jarBytes = this.classBuilder.createJarFromName("JarDeployerDUnitDNUWNC");
    -    JarClassLoader jarClassLoader =
    -        jarDeployer.deploy(new String[] {"JarDeployerDUnit2.jar"}, new byte[][] {jarBytes})[0];
    -    File deployedJar = new File(jarClassLoader.getFileCanonicalPath());
    -
    -    assertThat(deployedJar).exists();
    -    assertThat(deployedJar.getName()).contains("#1");
    -    JarClassLoader newJarClassLoader =
    -        jarDeployer.deploy(new String[] {"JarDeployerDUnit2.jar"}, new byte[][] {jarBytes})[0];
    -    assertThat(newJarClassLoader).isNull();
    -  }
    -
    -  @Test
       public void testDeployToInvalidDirectory() throws IOException, ClassNotFoundException {
    --- End diff --
    
    Will change this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode issue #429: Geode-2686

Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on the issue:

    https://github.com/apache/geode/pull/429
  
    It would be a good idea to do a walkthrough of these changes for us to understand the major change points. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode issue #429: Geode-2686

Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on the issue:

    https://github.com/apache/geode/pull/429
  
    Precheckin is started (still running)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107194282
  
    --- Diff: geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java ---
    @@ -126,14 +111,16 @@ public void testDeployToInvalidDirectory() throws IOException, ClassNotFoundExce
           public void run() {
             try {
               barrier.await();
    -        } catch (Exception e) {
    -          fail(e);
    +        } catch (InterruptedException iex) {
    --- End diff --
    
    This wasn't a test I wrote, it's been around for awhile.  I pulled it out of a DUnit into this class in 917dfa0.  It wasn't actually clear to me what the author was trying to test here, so I left it around intending to come back to it later.  Do you have any idea of what the author was trying to test?  The name of the test (testDeployToInvalidDirectory) seems unrelated to Concurrency.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart closed the pull request at:

    https://github.com/apache/geode/pull/429


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107045761
  
    --- Diff: geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java ---
    @@ -67,18 +68,16 @@ public static void deployJarsReceivedFromClusterConfiguration(Cache cache,
         String[] jarFileNames = response.getJarNames();
         byte[][] jarBytes = response.getJars();
     
    -    final JarDeployer jarDeployer = new JarDeployer(
    -        ((GemFireCacheImpl) cache).getDistributedSystem().getConfig().getDeployWorkingDir());
    -
         /******
          * Un-deploy the existing jars, deployed during cache creation, do not delete anything
          */
     
         if (jarFileNames != null && jarBytes != null) {
    -      JarClassLoader[] jarClassLoaders = jarDeployer.deploy(jarFileNames, jarBytes);
    +      List<DeployedJar> jarClassLoaders =
    +          ClassPathLoader.getLatest().getJarDeployer().deploy(jarFileNames, jarBytes);
           for (int i = 0; i < jarFileNames.length; i++) {
    -        if (jarClassLoaders[i] != null) {
    -          logger.info("Deployed " + (jarClassLoaders[i].getFileCanonicalPath()));
    +        if (jarClassLoaders.get(i) != null) {
    --- End diff --
    
    Is the jarClassLoaders backed live by ClassPathLoader such that another thread could be changing the List while this thread is in this for loop?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107195082
  
    --- Diff: geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java ---
    @@ -144,40 +131,99 @@ public void run() {
           Thread.sleep(500);
           alternateDir.mkdir();
           thread.join();
    -    } catch (Exception e) {
    -      fail(e);
    +    } catch (InterruptedException iex) {
    +      fail("Interrupted while waiting.");
    --- End diff --
    
    I made sure to avoid this antipattern in the tests I added, but I had left some old tests around unchanged.  I'll go back and clean them up today.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode issue #429: Geode-2686

Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on the issue:

    https://github.com/apache/geode/pull/429
  
    Closing this PR until GEODE-2705 and a few other concerns are addressed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107192442
  
    --- Diff: geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java ---
    @@ -67,18 +68,16 @@ public static void deployJarsReceivedFromClusterConfiguration(Cache cache,
         String[] jarFileNames = response.getJarNames();
         byte[][] jarBytes = response.getJars();
     
    -    final JarDeployer jarDeployer = new JarDeployer(
    -        ((GemFireCacheImpl) cache).getDistributedSystem().getConfig().getDeployWorkingDir());
    -
         /******
          * Un-deploy the existing jars, deployed during cache creation, do not delete anything
          */
     
         if (jarFileNames != null && jarBytes != null) {
    -      JarClassLoader[] jarClassLoaders = jarDeployer.deploy(jarFileNames, jarBytes);
    +      List<DeployedJar> jarClassLoaders =
    +          ClassPathLoader.getLatest().getJarDeployer().deploy(jarFileNames, jarBytes);
           for (int i = 0; i < jarFileNames.length; i++) {
    -        if (jarClassLoaders[i] != null) {
    -          logger.info("Deployed " + (jarClassLoaders[i].getFileCanonicalPath()));
    +        if (jarClassLoaders.get(i) != null) {
    --- End diff --
    
    This loop is just printing the jars that got deployed from ClusterConfiguration on member startup.  If another thread deploys a jar in the middle of this loop, this list won't be updated to  include that jar.  But I believe that is the desired behavior.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107195612
  
    --- Diff: geode-core/src/test/java/org/apache/geode/internal/ClassPathLoaderTest.java ---
    @@ -65,6 +65,26 @@ public void testLatestExists() throws Exception {
         assertNotNull(ClassPathLoader.getLatest());
       }
     
    +  @Test
    +  public void testZeroLengthFile() throws IOException, ClassNotFoundException {
    +    try {
    +      ClassPathLoader.getLatest().getJarDeployer().deploy(new String[] {"JarDeployerDUnitZLF.jar"},
    +          new byte[][] {new byte[0]});
    +      fail("Zero length files are not deployable");
    +    } catch (IllegalArgumentException expected) {
    --- End diff --
    
    I'll clean this up.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by jaredjstewart <gi...@git.apache.org>.
Github user jaredjstewart commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107221701
  
    --- Diff: geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java ---
    @@ -126,14 +111,16 @@ public void testDeployToInvalidDirectory() throws IOException, ClassNotFoundExce
           public void run() {
             try {
               barrier.await();
    -        } catch (Exception e) {
    -          fail(e);
    +        } catch (InterruptedException iex) {
    --- End diff --
    
    Do you have an idea for how to avoid this line?
    
    ```
    Thread.sleep(500) you have commented with `//don't ever do this
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107048816
  
    --- Diff: geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java ---
    @@ -126,14 +111,16 @@ public void testDeployToInvalidDirectory() throws IOException, ClassNotFoundExce
           public void run() {
             try {
               barrier.await();
    -        } catch (Exception e) {
    -          fail(e);
    +        } catch (InterruptedException iex) {
    --- End diff --
    
    Never catch "unexpected" exceptions in a JUnit Test. This makes a mess and adds tons of lines to a test for no good reason. JUnit framework handles "unexpected" exceptions than this crazy catch/rethrow business. All the books about JUnit say don't do this. Also, use new Concurrency constructs.
    ```java
        Future<Boolean> done = Executors.newSingleThreadExecutor().submit(() -> {
          barrier.await();
          jarDeployer.deployWithoutRegistering("JarDeployerIntegrationTest.jar", jarBytes);
          return true;
        });
    
        barrier.await();
        Thread.sleep(500); // don't ever do this
        alternateDir.mkdir();
        assertThat(done.get(2, TimeUnit.MINUTES)).isTrue();
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #429: Geode-2686

Posted by kirklund <gi...@git.apache.org>.
Github user kirklund commented on a diff in the pull request:

    https://github.com/apache/geode/pull/429#discussion_r107046157
  
    --- Diff: geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java ---
    @@ -39,83 +44,63 @@
       @Rule
       public TemporaryFolder temporaryFolder = new TemporaryFolder();
     
    -  @Rule
    -  public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
    +  JarDeployer jarDeployer;
     
       @Before
       public void setup() {
    -    System.setProperty("user.dir", temporaryFolder.getRoot().getAbsolutePath());
         classBuilder = new ClassBuilder();
    -    ClassPathLoader.setLatestToDefault();
    +    jarDeployer = new JarDeployer(temporaryFolder.getRoot());
       }
     
    -  @Test
    -  public void testDeployFileAndChange() throws Exception {
    -    final JarDeployer jarDeployer = new JarDeployer();
    +  private byte[] createJarWithClass(String className) throws IOException {
    +    String stringBuilder = "package integration.parent;" + "public class " + className + " {}";
     
    -    // First deploy of the JAR file
    -    byte[] jarBytes = this.classBuilder.createJarFromName("ClassA");
    -    JarClassLoader jarClassLoader =
    -        jarDeployer.deploy(new String[] {"JarDeployerDUnit.jar"}, new byte[][] {jarBytes})[0];
    -    File deployedJar = new File(jarClassLoader.getFileCanonicalPath());
    +    return this.classBuilder.createJarFromClassContent("integration/parent/" + className,
    +        stringBuilder);
    +  }
     
    -    assertThat(deployedJar).exists();
    -    assertThat(deployedJar.getName()).contains("#1");
    -    assertThat(deployedJar.getName()).doesNotContain("#2");
    +  @Test
    +  public void testFileVersioning() throws IOException, ClassNotFoundException {
    +    String jarName = "JarDeployerIntegrationTest.jar";
     
    -    assertThat(ClassPathLoader.getLatest().forName("ClassA")).isNotNull();
    +    byte[] firstJarBytes = createJarWithClass("ClassA");
     
    -    assertThat(doesFileMatchBytes(deployedJar, jarBytes));
    +    // First deploy of the JAR file
    +    DeployedJar firstDeployedJar = jarDeployer.deployWithoutRegistering(jarName, firstJarBytes);
     
    -    // Now deploy an updated JAR file and make sure that the next version of the JAR file
    -    // was created and the first one was deleted.
    -    jarBytes = this.classBuilder.createJarFromName("ClassB");
    -    JarClassLoader newJarClassLoader =
    -        jarDeployer.deploy(new String[] {"JarDeployerDUnit.jar"}, new byte[][] {jarBytes})[0];
    -    File nextDeployedJar = new File(newJarClassLoader.getFileCanonicalPath());
    +    assertThat(firstDeployedJar.getFile()).exists().hasBinaryContent(firstJarBytes);
    +    assertThat(firstDeployedJar.getFile().getName()).contains(".v1.").doesNotContain(".v2.");
     
    -    assertThat(nextDeployedJar.exists());
    -    assertThat(nextDeployedJar.getName()).contains("#2");
    -    assertThat(doesFileMatchBytes(nextDeployedJar, jarBytes));
    +    // Now deploy an updated JAR file and make sure that the next version of the JAR file
    +    // was created
    +    byte[] secondJarBytes = createJarWithClass("ClassB");
     
    -    assertThat(ClassPathLoader.getLatest().forName("ClassB")).isNotNull();
    +    DeployedJar secondDeployedJar = jarDeployer.deployWithoutRegistering(jarName, secondJarBytes);
    +    File secondDeployedJarFile = new File(secondDeployedJar.getFileCanonicalPath());
     
    -    assertThatThrownBy(() -> ClassPathLoader.getLatest().forName("ClassA"))
    -        .isExactlyInstanceOf(ClassNotFoundException.class);
    +    assertThat(secondDeployedJarFile).exists().hasBinaryContent(secondJarBytes);
    +    assertThat(secondDeployedJarFile.getName()).contains(".v2.").doesNotContain(".v1.");
     
    -    assertThat(jarDeployer.findSortedOldVersionsOfJar("JarDeployerDUnit.jar")).hasSize(1);
    +    File[] sortedOldJars = jarDeployer.findSortedOldVersionsOfJar(jarName);
    +    assertThat(sortedOldJars).hasSize(2);
    +    assertThat(sortedOldJars[0].getName()).contains(".v2.");
    +    assertThat(sortedOldJars[1].getName()).contains(".v1.");
         assertThat(jarDeployer.findDistinctDeployedJars()).hasSize(1);
       }
     
       @Test
    -  public void testDeployNoUpdateWhenNoChange() throws Exception {
    -    final JarDeployer jarDeployer = new JarDeployer();
    -
    -    // First deploy of the JAR file
    -    byte[] jarBytes = this.classBuilder.createJarFromName("JarDeployerDUnitDNUWNC");
    -    JarClassLoader jarClassLoader =
    -        jarDeployer.deploy(new String[] {"JarDeployerDUnit2.jar"}, new byte[][] {jarBytes})[0];
    -    File deployedJar = new File(jarClassLoader.getFileCanonicalPath());
    -
    -    assertThat(deployedJar).exists();
    -    assertThat(deployedJar.getName()).contains("#1");
    -    JarClassLoader newJarClassLoader =
    -        jarDeployer.deploy(new String[] {"JarDeployerDUnit2.jar"}, new byte[][] {jarBytes})[0];
    -    assertThat(newJarClassLoader).isNull();
    -  }
    -
    -  @Test
       public void testDeployToInvalidDirectory() throws IOException, ClassNotFoundException {
    --- End diff --
    
    Change this to "throws Exception" -- that's the standard best practice for a @Test method in JUnit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---