Address the issue #796 #798
Conversation
|
@tjquinno @donbourne @Channyboy further to the Technical call discussion, I have proposed this PR. With this, you can go ahead to perform a micro release. It should not have any impact on the existing implementations. |
There was a problem hiding this comment.
I am requesting some minor edits noted with the source changes.
Also, some might think it excessive but consider adding a build-time test class in src/test/java which extends Snapshot and does not provide an implementation for bucketValues. The normal Maven compilation of the test directory and this new class would ensure that the changes in the PR address the underlying issue now and going forward. (There would be no point in adding a TCK test for this because we want to test the behavior of the MP Metrics API library itself, not implementations of the MP Metrics spec.)
| @@ -36,6 +36,10 @@ | |||
| <groupId>org.osgi</groupId> | |||
| <artifactId>org.osgi.annotation.versioning</artifactId> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>biz.aQute.bnd</groupId> | |||
| <artifactId>bnd-baseline-maven-plugin</artifactId> | |||
There was a problem hiding this comment.
IIUC this dependency needed to process the new use of the @BaselineIgnore annotation in the source code.
The annotation has retention CLASS (not RUNTIME) so there is no runtime detection of the annotation.
Perhaps, therefore, this dependency should have scope provided to avoid the plug-in bleeding into the runtime class path of anything that depends on the MP Metrics API component.
There was a problem hiding this comment.
I cloned Emily's fork and tried to push a commit to the fix branch to add <scope>provided</scope> but I do not have permission to write.
| @@ -64,7 +64,10 @@ public abstract class Snapshot { | |||
| * | |||
| * @return an array of {@link HistogramBucket} if it is available or an empty array if not available | |||
| */ | |||
| public abstract HistogramBucket[] bucketValues(); | |||
| @aQute.bnd.annotation.baseline.BaselineIgnore("5.2") | |||
There was a problem hiding this comment.
My mistake. the "5.2" is also a reasonable future version of MP Metrics to specify.
Why is the baseline package version "5.2" in the annotation?
I know that is the version of the plug-in being used, but the Javadoc for the value attribute for the annotation states
Baseline package version.
The version must be a valid OSGi version string.
That doesn't clarify much, but It seems that this value is intended to reflect the version of the component being produced, not the version of the bnd baseline plug-in being used.
|
Here is a candidate subsequent PR: #799 |
This is to fix the issue #796 by providing an implementation to the abstract method and also skip this change from semantic versioning check.