Ok, makes sense. Den tor. 15. aug. 2019 kl. 17.16 skrev Ethan Li <[email protected]>:
> It was added for backwards compatibility. > > Apache Storm 2.x supports running older version of workers. So users can > submit 1.x topology on a 2.x Storm cluster. We have been using it in this > way until we move all our users to use 2.x topology. This is nice feature > so that users can migrate gradually. > > To support this, Apache Storm needs to be set up with > *supervisor.worker.version.classpath.map *config which tells supervisors > where to find the classes for this specific version of topology . If the > topology is 1.x, there is no storm-client jar. The version info is found in > storm-core jar. That’s why it’s introduced. We should probably keep it. > > > > > On Aug 15, 2019, at 9:57 AM, Stig Rohde Døssing <[email protected]> > wrote: > > Oops, was wrong. The revision stuff is also being read by Storm UI. > > Yes, we should just fix VersionInfoMojo to handle running outside SCM. I'll > put up a PR to fix it shortly. I think we can also delete the version info > file in storm-core. I can't think of any reason why storm-core would be on > the classpath, but storm-client wouldn't be. > > Den tor. 15. aug. 2019 kl. 16.24 skrev Ethan Li <[email protected] > >: > > Sorry for typos. I meant "If it’s not built within git or SCM" > > On Aug 15, 2019, at 9:23 AM, Ethan Li <[email protected]> wrote: > > I prefer to revert the change in VersionInfoMojo. These fields > > version-info.properties are introduced on purpose in > https://github.com/apache/storm/pull/2844 < > https://github.com/apache/storm/pull/2844> > https://github.com/apache/storm/pull/2891 < > https://github.com/apache/storm/pull/2891> so we should probably keep it. > > > It makes sense to return unknown if it’s built within git or SCM as > > there is no other way to tell. > > > > On Aug 15, 2019, at 9:12 AM, Derek Dagit <[email protected] <mailto: > > [email protected]>> wrote: > > > Well, the change seems to be to throw an IllegalArgumentException > instead of what was the default behavior: to return a String "Unknown". > > It is unclear how it would work outside of a Git working tree (as when > the source is built from a download), so it seems the easiest fix would > be to change the default labels to do nothing and revert to the previous > behavior—if we can convince checkstyle that it is OK. > > A better fix might be as you suggested. I like the idea of the build not > behaving differently depending on if it is executed within SCM, at least > if we can help it. > > In any case, I am inclined to think that this should be fixed before the > release, as it is a bug that prevents the code from building as per our > instructions. > > On Thu, Aug 15, 2019 at 02:08:17PM +0200, Stig Rohde Døssing wrote: > > > As far as I can tell, the version plugin is supposed to populate these > files > > > https://github.com/apache/storm/blob/master/storm-client/src/resources/storm-client-version-info.properties > < > > https://github.com/apache/storm/blob/master/storm-client/src/resources/storm-client-version-info.properties > > > and > > > https://github.com/apache/storm/blob/master/storm-core/src/resources/storm-core-version-info.properties > . > > I'm not sure why we need this file in both storm-client and > > storm-core, I'm > > not sure anything reads the storm-core file, so maybe we can delete it? > > When the build runs, those files are supposed to be populated with > > version > > info (commit hash, author info and so on) from the plugin. The version > > info > > is then read into > > > https://github.com/apache/storm/blob/925422a5b5ad1c3329a2c2b44db460ae94f70806/storm-client/src/jvm/org/apache/storm/utils/VersionInfo.java > . > > The VersionInfo file also loads the Storm version from the pom, where > > it's > > used to populate e.g. the version in Storm UI for Nimbus and > > Supervisors. > > > It looks like this: > > version=2.0.0 > revision=2ba95bbd1c911d4fc6363b1c4b9c4c6d86ac9aae > branch=(no branch) > user=tgoetz > date=2019-04-29T21:17Z > url=https://git-wip-us.apache.org/repos/asf/storm.git > srcChecksum=58779abf76d8a8bbd20a2bd5f466 > > The only field I see read is the Storm version from the pom, I'm not > > sure > > we use the other stuff (e.g. commit hash, commit author). For 2.0.0 > something broke when splitting out storm-client, so the file for that > module doesn't have all fields populated: > > version=2.0.0 > revision=${version-info.scm.commit} > branch=${version-info.scm.branch} > user=tgoetz > date=${version-info.build.time} > url=${version-info.scm.uri} > srcChecksum=${version-info.source.md5} > > It's most likely just because storm-client is missing configuration to > > run > > the plugin when building. > > I think if we're not using these extra fields from git, we might as > > well > > drop the version info mojo. If we only need to set the "version" > > field, we > > don't need to invoke git during the build. > > If we do need these fields for something, we should just make the mojo > > set > > the fields to "Unknown" if it fails to invoke git (i.e. if SCM.NONE is > set). I'm guessing what's happening for Derek is that he downloaded the > sources and is trying to run the build with files that aren't part of > > a git > > repo. So when the mojo tries to do "git branch" it gets an error. > > What do you think? > > Den tor. 15. aug. 2019 kl. 06.02 skrev Ethan Li < > > [email protected]>: > > > Derek ran into this error when building from source: > > [ERROR] Failed to execute goal > org.apache.storm:storm-maven-plugins:2.1.0:version-info > > (version-info) on > > project storm-core: java.lang.IllegalArgumentException: SCM NONE is > > not > > supported -> [Help 1] > > > The log before this is > > [INFO] --- storm-maven-plugins:2.1.0:version-info (version-info) @ > storm-core --- > [WARNING] [svn, info] failed with error code 1 > [WARNING] [git, branch] failed with error code 128 > [INFO] SCM: NONE > > > I compared with apache-storm-2.0.0 and found that it can pass although > there are similar WARNINGs: > > [INFO] --- storm-maven-plugins:2.0.0:version-info (version-info) @ > storm-core --- > [WARNING] [svn, info] failed with error code 1 > [WARNING] [git, branch] failed with error code 128 > [INFO] SCM: NONE > [INFO] Computed MD5: 58779abf76d8a8bbd20a2bd5f466 > > This is the PR that changed the behavior > > > https://github.com/apache/storm/pull/3061/files#diff-f1197661326b04376e94e3b25cc3eedcR197 > > < > > > https://github.com/apache/storm/pull/3061/files#diff-f1197661326b04376e94e3b25cc3eedcR197 > > . > > I am not sure what this storm-maven-plugin should do though. Will > > need to > > look into it. Or if anyone has better idea, please let me know. > > > > On Aug 14, 2019, at 6:53 PM, P. Taylor Goetz <[email protected]> > > wrote: > > > Nice work Ethan! > > I’ll try to take a look in the next few days. > > -Taylor > > On Aug 14, 2019, at 2:13 PM, Ethan Li <[email protected]> wrote: > > This is a call to vote on releasing Apache Storm 2.1.0 (rc2) > > Full list of changes in this release: > > > > > https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.1.0-rc2/RELEASE_NOTES.html > > > The tag/commit to be voted upon is v2.1.0: > > > > > https://gitbox.apache.org/repos/asf?p=storm.git;a=tree;h=6c7adb8c7095cb718d5c49d07331034ae0e8434a;hb=f875619fc1017e401d65bd2900b195738f90e754 > > > The source archive being voted upon can be found here: > > > > > https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.1.0-rc2/apache-storm-2.1.0-src.tar.gz > > > Other release files, signatures and digests can be found here: > > https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.1.0-rc2 > > The release artifacts are signed with the following key: > > https://www.apache.org/dist/storm/KEYS > > The Nexus staging repository for this release is: > > > https://repository.apache.org/content/repositories/orgapachestorm-1085/ > > > Please vote on releasing this package as Apache Storm 2.1.0. > > When voting, please list the actions taken to verify the release. > > This vote will be open for at least 72 hours. > > [ ] +1 Release this package as Apache Storm 2.1.0 > [ ] 0 No opinion > [ ] -1 Do not release this package because... > > Thanks to everyone who contributed to this release. > > Ethan > > >
