Jinmei, thank you for that. Whilst I believe supporting existing users custom
formats sound nice, it does complicate the whole process too much.
As can be seen from the example code that I posted, the test is already
retrieving the wrong artifact name, as there is no way to determine what the
correct format should be if there are many.
I think it is reasonable to standardize on a format. Document said format and
then apply the pattern consistently. Then adding that to the notes.
It even simplifies the logic where we have to deal with non-versioned jars and
we then append the ".v + {version}" to make sure we can handle versions, etc...
As this can now be adapted to "abc.jar" being converted to "abc-1.0.0.jar".
Thus being able to add a version to an unversioned jar. In addition a standard
way to determine a version,etc...
As I said, unfortunately the is no simple way to deal with this transition and
transitioning to a standard by ripping off the bandaid in one go is sometime
preferable.
--Udo
On 10/8/20, 9:25 AM, "Jinmei Liao" <[email protected]> wrote:
Wait, that reason doesn't make much sense either. Dale/Darrel, do you
remember why we did what we did?
On 10/7/20, 3:12 PM, "Jinmei Liao" <[email protected]> wrote:
I believe we did this for a reason, can't remember exactly what though.
Most probably drive by user's existing filenames. I believe we are probably
concerned that user's jar name might contain "_" or "-" themselves, like
common-logging.jar etc. So we had to resort to finding the first "." followed
by a digit to determine where the version pattern begins.
On 10/7/20, 1:44 PM, "Udo Kohlmeyer" <[email protected]> wrote:
Hi there Geode Dev List,
Whilst doing work on
GEODE-8466<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissues.apache.org%2Fjira%2Fbrowse%2FGEODE-8466&data=02%7C01%7Cudo%40vmware.com%7C1393e334db5440233f5108d86b0fd4c6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637377063031142340&sdata=tTX%2FDQcFdz8U%2BbMgtqgnVAC2cA6J669NP8nqF5b2kyg%3D&reserved=0>
and looking at the functionality that the
ClassPathLoader.java<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fblob%2Fdevelop%2Fgeode-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgeode%2Finternal%2FClassPathLoader.java&data=02%7C01%7Cudo%40vmware.com%7C1393e334db5440233f5108d86b0fd4c6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637377063031142340&sdata=6lfD1hfG%2BMpshoP3zgykVlPYi6rcP5OWAHjDBxv2jo8%3D&reserved=0>,
JarDeployer.java<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fblob%2Fdevelop%2Fgeode-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgeode%2Finternal%2FJarDeployer.java&data=02%7C01%7Cudo%40vmware.com%7C1393e334db5440233f5108d86b0fd4c6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637377063031142340&sdata=jjf9KSmHTQSJh1DMEbG8fDcKpnVDldP8ZBzH5Tz1a%2Bc%3D&reserved=0>
and
DeployedJar.java<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fblob%2Fdevelop%2Fgeode-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgeode%2Finternal%2FDeployedJar.java&data=02%7C01%7Cudo%40vmware.com%7C1393e334db5440233f5108d86b0fd4c6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637377063031152342&sdata=nQU2zcCFbWpiGhUwQEI89lYz5jbp4VjPkowRqy%2Fvpa4%3D&reserved=0>
provide around the “Deploy Jar” functionality, we came across some interesting
“supported” filename patterns.
According to the
JarDeployerFileTest.java<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fblob%2Fdevelop%2Fgeode-core%2Fsrc%2FintegrationTest%2Fjava%2Forg%2Fapache%2Fgeode%2Finternal%2FJarDeployerFileTest.java&data=02%7C01%7Cudo%40vmware.com%7C1393e334db5440233f5108d86b0fd4c6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637377063031152342&sdata=SD6wNi7o6Fk5mPQAa%2B141u8MkK%2Bh1j6d2sgdnzjfkMI%3D&reserved=0>
the “supported” formats are as follows:
assertThat(JarDeployer.getArtifactId("abc.jar")).isEqualTo("abc");
assertThat(JarDeployer.getArtifactId("abc-1.jar")).isEqualTo("abc");
assertThat(JarDeployer.getArtifactId("ab.c.1.jar")).isEqualTo("ab.c");
assertThat(JarDeployer.getArtifactId("abc.v1.jar")).isEqualTo("abc.v1");
assertThat(JarDeployer.getArtifactId("abc-1.0.snapshot.jar")).isEqualTo("abc");
assertThat(JarDeployer.getArtifactId("abc-1.0.v1.jar")).isEqualTo("abc");
assertThat(JarDeployer.getArtifactId("spark-network-common_2.11-2.3.1.jar"))
.isEqualTo("spark-network-common_2");
Which don’t make any sense. As the generally accepted norm for a
version jar file would be: “<artifact name>[ - <major> . <minor> . <patch> -
<Release Tag> ] .jar”. (note the syntax in red)
I want to suggest that we DISCONTINUE supporting all jar name
formats other than the one mentioned above IMMEDIATELY. As the supported name
format is just “funky” but also wrong and can lead to misclassification of the
artifact name…. as some of you with a keen eye would have spotted already 😉
For those who did not spot the mistake…
“spark-network-common_2.11-2.3.1.jar” is incorrectly classified and has the
WRONG artifact name. As “spark-network-common_2.11” is the correct artifact
name NOT “spark-network-common_2”!
I would like to introduce this change with
GEODE-8466<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissues.apache.org%2Fjira%2Fbrowse%2FGEODE-8466&data=02%7C01%7Cudo%40vmware.com%7C1393e334db5440233f5108d86b0fd4c6%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637377063031152342&sdata=kBrNxX9m%2BkOVheEcxSlMZq1%2F1RJQut8EgbYUhdo5WZQ%3D&reserved=0>.
This would be a “breaking” change, but we should change this sooner than
later. There is no transition ability here, as it would be too hard to have
Geode support both, as there is no simple way for the system to decide if the
name conforms to the “correct” format or not.
DISCUSS!!!
--Udo
