[WIP] clean up startup logging around catalog #856
Conversation
| @@ -103,8 +103,8 @@ private TemporaryInternalScanResult scanForCatalogInternal(Bundle bundle, boolea | |||
| this.managementContext.getCatalog().addTypesFromBundleBom(bomText, mb, force, result.mapOfNewToReplaced); | |||
| if (validate) { | |||
| Set<RegisteredType> matches = MutableSet.copyOf(this.managementContext.getTypeRegistry().getMatching(RegisteredTypePredicates.containingBundle(mb.getVersionedName()))); | |||
| if (!(matches.containsAll(result.mapOfNewToReplaced.keySet()) && result.mapOfNewToReplaced.keySet().containsAll(matches))) { | |||
| // sanity check | |||
| if (matches.size()!=result.mapOfNewToReplaced.size()) { | |||
There was a problem hiding this comment.
I'm afraid this will be too loose: the 2 sets have the same size but we don't know if the items are "the same", i.e. same symbolicName and version.
Could we have a middle ground here? Something like looping over each items in result.mapOfNewToReplaced, checking if matches contains one with the same symbolicName and version. Those fields should be resolved and would provide sufficient equality in this case. WDYT?
There was a problem hiding this comment.
@ahgittin can you explain more the purpose/context of this check, and why you only want to log.warn if the sizes are different?
I recall seeing such warnings in the log when I suspect we shouldn't have, so think you're right to guard against it better.
For @tbouron's suggestion of checking the symbolicName:version, that makes sense - or does this get called a lot or with really big things so performance matters?
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Does not seem in the scope of this PR. Also not sure why deprecating it?
There was a problem hiding this comment.
Looks like code is not called, and it's in an internal package. Therefore I'm ahppy with it being deprecated (or even deleted immediately). Same for below.
| * @param item Catalog item to remove | ||
| * @deprecated since 0.13.0 remove bundles | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Does not seem in the scope of this PR. Also not sure why deprecating it?
| * | ||
| * @param item Catalog items to remove | ||
| * @param item Catalog item to remove | ||
| * @deprecated since 0.13.0 remove bundles |
| * Remove the given item from the catalog. | ||
| * | ||
| * @param item Catalog item to remove | ||
| * @deprecated since 0.13.0 remove bundles |
|
@ahgittin Do you think you can look at the comments above? |
|
@ahgittin Any news on this PR? |
|
@ahgittin Can you take a look at this please to see if it is still relevant, and address the comments above if appropriate Thanks |
just looking at logs and tidying up stuff that is wrong or misleading
have done one so far but i think there are others. best hold off reviewing until all done.