Clean up inference around parsing `name:version` strings #737

ahgittin · 2017-06-16T16:27:12Z

We've had some rough heuristics how to find name and version but not a strict definition.

This adds strict definitions in RegisteredTypeNaming (first commit).

It then changes core to prefer these methods. It doesn't change behaviour (except in a few fringe cases noted), but it does add log warnings where we were relying on hokey old strategies (previously assumed something was a version if it started with a number, that's it).

Then clean up a bunch of deprecated stuff where the above was used.

The methods in RegisteredTypeNaming let us hedge a little bit whether we want to be strict with OSGi versions or not. That's a subject for another PR (but an important one as discussed at #672).

…LikeVersionedId`

changed a while ago to EntitySpecResolver; removes references to catalog items and hokey lookup

ahgittin · 2017-06-16T16:27:33Z

(There is a failing test, I'm working on that, otherwise this is good to review.)

ahgittin · 2017-06-20T09:00:30Z

@geomacy @neykov ready for review. does not do the version changes discussed on ML but prepares us for them (and helped me understand them). this is mainly to fix issues where people have used funny v1 version strings.

ahgittin · 2017-06-20T09:00:50Z

(the test failure is a test env config issue in hudson remoting)

ahgittin · 2017-06-21T19:00:10Z

re tests - see #740 which merges this and test pass

geomacy · 2017-06-22T16:40:04Z

@ahgittin haven't ignored this but haven't been able to find the bandwidth to look at it - will be off tomorrow, but will look at this and related PRs on Monday!

geomacy

Broadly this looks good but I don't feel I've got to the bottom of it yet - I have a question in particular about the code in BasicBrooklynCatalog.attemptType, see my comment there, could you add some detail around that?

geomacy · 2017-06-26T13:49:28Z

core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java

+    /** 
+     * For type names we currently work with any non-empty string that does not contain 
+     * a ':' or whitespace or forward slash or backslash.
+     * However we discourage things that are not OSGi symbolic names; see {@link #isValidTypeName(String)}. 


isGoodTypeName not isValidTypeName

geomacy · 2017-06-26T13:53:21Z

core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypeNaming.java

+    public final static String QUALIFIER = OSGI_TOKEN_REGEX;
+    public final static String VERSION_REGEX = 
+        NUMBER + 
+            "(" + "\\." + NUMBER +  


You've already defined DOT, might as well use it here.

geomacy · 2017-06-26T14:08:22Z

...n/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java

@@ -82,7 +79,6 @@
 * This generates instances of a template resolver that use a {@link ServiceTypeResolver}


Best to update description now ServiceTypeResolver is deleted. Should probably also remove this now too.

geomacy · 2017-06-26T14:33:21Z

core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java

@@ -601,14 +602,34 @@ private void collectCatalogItems(String sourceYaml, Map<?,?> itemMetadata, List<
        // if symname not set, infer from: id, then name, then item id, then item name
        if (Strings.isBlank(symbolicName)) {


The changes in this block and the one below on versions are really pushing the line number count of this method, would be nice to split them out into another method; could it even be a method of RegisteredTypeNaming?

want to completely gut this class as soon as #363 YOML is available, and remove much of this once 0.12.0 is out -- refactoring i think would make that harder as well as of course not being worth the effort if we're gutting it

geomacy · 2017-06-26T14:59:26Z

core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java

+                        if (candidateCiType == candidate.getCatalogItemType() &&
+                                (type.equals(candidate.getSymbolicName()) || type.equals(candidate.getId()))) {
+                            if (version==null || version.equals(candidate.getVersion())) {
+                                log.warn("Lookup of '"+type+"' version '"+version+"' only worked using legacy routines; please advise Brooklyn community so they understand why");


Maybe we should even make this an error to highlight it more?

geomacy · 2017-06-26T16:35:31Z

core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java

+            }
+            {
+                // legacy routine; should be the same as above code added in 0.12 because:
+                // if type is symbolic_name, the type will match below, and version will be null so any version allowed to match 


I'm afraid I don't follow this - is it the case that a type which is just a symbolic_name will be taken care of by line 900 above (so you actually mean "the type will match above" on this line i.e. at line 900); but how is the next case handled, where type is of form symbolic_name:version? When you say the id will match, where does that happen?

yes, should be above

grand, but how about the question above where type is of form symbolic_name:version? When you say "the id will match", where does that happen?

geomacy · 2017-06-26T16:41:18Z

Would also be interested in @neykov's comments on this!

neykov · 2017-06-27T07:40:37Z

@geomacy left my comments in #740 which includes this PR.

geomacy · 2017-06-27T08:21:51Z

👍 I was going to look at that next :-)

ahgittin · 2017-06-28T10:46:01Z

great comments here @geomacy , addressed all except where commented above; moving on to #740

ahgittin added 5 commits June 16, 2017 09:49

remove REST reference to removed method

36ffdee

new utilities for registered type (catalog item id) naming syntax

3860c34

move many things to use RegisteredTypeNaming rather than hokey `looks…

607fc85

…LikeVersionedId`

notes on related methods for looking up catalog items flexibly

eb08f8e

remove deprecated CAMP ServiceTypeResolver

c9d03c9

changed a while ago to EntitySpecResolver; removes references to catalog items and hokey lookup

ahgittin added 2 commits June 20, 2017 09:37

fix tests that relied on removed deprecated behaviour

db77f4e

Merge branch 'master' into version-id

dc4942a

ahgittin changed the title ~~[WIP] Clean up inference around parsing name:version strings~~ Clean up inference around parsing name:version strings Jun 20, 2017

ahgittin added a commit to ahgittin/brooklyn-server that referenced this pull request Jun 21, 2017

This closes apache#737

b46188e

ahgittin mentioned this pull request Jun 21, 2017

New versioning rules prep #740

Merged

ahgittin added a commit to ahgittin/brooklyn-server that referenced this pull request Jun 22, 2017

This closes apache#737

c5c5afc

ahgittin mentioned this pull request Jun 22, 2017

Version new rules #743

Merged

geomacy reviewed Jun 26, 2017

View reviewed changes

address comments on PR in apache#737

0236dbc

asfgit merged commit 0236dbc into apache:master Jun 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up inference around parsing `name:version` strings #737

Clean up inference around parsing `name:version` strings #737

ahgittin commented Jun 16, 2017

ahgittin commented Jun 16, 2017

ahgittin commented Jun 20, 2017

ahgittin commented Jun 20, 2017

ahgittin commented Jun 21, 2017

geomacy commented Jun 22, 2017

geomacy left a comment

geomacy Jun 26, 2017

geomacy Jun 26, 2017

geomacy Jun 26, 2017

geomacy Jun 26, 2017

ahgittin Jun 28, 2017

geomacy Jun 26, 2017

geomacy Jun 26, 2017

ahgittin Jun 28, 2017

geomacy Jun 28, 2017

geomacy commented Jun 26, 2017

neykov commented Jun 27, 2017

geomacy commented Jun 27, 2017

ahgittin commented Jun 28, 2017

		@@ -82,7 +79,6 @@
		* This generates instances of a template resolver that use a {@link ServiceTypeResolver}

		@@ -601,14 +602,34 @@ private void collectCatalogItems(String sourceYaml, Map<?,?> itemMetadata, List<
		// if symname not set, infer from: id, then name, then item id, then item name
		if (Strings.isBlank(symbolicName)) {

Clean up inference around parsing name:version strings #737

Clean up inference around parsing name:version strings #737

Conversation

ahgittin commented Jun 16, 2017

ahgittin commented Jun 16, 2017

ahgittin commented Jun 20, 2017

ahgittin commented Jun 20, 2017

ahgittin commented Jun 21, 2017

geomacy commented Jun 22, 2017

geomacy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geomacy commented Jun 26, 2017

neykov commented Jun 27, 2017

geomacy commented Jun 27, 2017

ahgittin commented Jun 28, 2017

Clean up inference around parsing `name:version` strings #737

Clean up inference around parsing `name:version` strings #737