This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 152392 - add annotation based registration of LookupProvider instances
Summary: add annotation based registration of LookupProvider instances
Status: RESOLVED FIXED
Alias: None
Product: projects
Classification: Unclassified
Component: Generic Infrastructure (show other bugs)
Version: 6.x
Hardware: All All
: P2 blocker (vote)
Assignee: Milos Kleint
URL:
Keywords: API, API_REVIEW_FAST
Depends on:
Blocks: 152984
  Show dependency tree
 
Reported: 2008-11-05 09:16 UTC by Milos Kleint
Modified: 2009-02-19 22:56 UTC (History)
1 user (show)

See Also:
Issue Type: TASK
Exception Reporter:


Attachments
apichange, new annotation (7.21 KB, patch)
2008-11-05 14:41 UTC, Milos Kleint
Details | Diff
modules rewritten to use annotation (47.14 KB, patch)
2008-11-05 14:41 UTC, Milos Kleint
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Milos Kleint 2008-11-05 09:16:47 UTC
see summary
Comment 1 Milos Kleint 2008-11-05 14:41:06 UTC
Created attachment 73306 [details]
apichange, new annotation
Comment 2 Milos Kleint 2008-11-05 14:41:47 UTC
Created attachment 73307 [details]
modules rewritten to use annotation
Comment 3 Milos Kleint 2008-11-05 14:44:09 UTC
please review this change that adds new annotation @LookupProvider.Register to register per-project instances of
LookupProvider interface.
included (in separate patch) are also the changes done to modules in main repository
Comment 4 Jesse Glick 2008-11-05 18:34:32 UTC
[JG01] The conventional name would be .Registration (a noun), not .Register (a verb). See

http://wiki.netbeans.org/DeclarativeRegistrationUsingAnnotations#section-DeclarativeRegistrationUsingAnnotations-NamingConventions


[JG02] websvc.jaxwsmodel/src/org/netbeans/modules/websvc/jaxwsmodel/resources/layer.xml should be deleted if empty.
Comment 5 Milos Kleint 2008-11-06 15:02:15 UTC
JG01, JG02: both done.
Comment 6 Milos Kleint 2008-11-10 07:15:57 UTC
thanks for review
Comment 7 Jaroslav Tulach 2008-11-10 07:57:22 UTC
Y01 I'd like changes that introduce new annotation processors to have associated unit test.

Y02 I've done search through all the modified usages of the new API and I think the API is not appropriate for 
majority of usages. The most common usecase (more than 30 times) seems to be to register object with 
constructor(Project) or constructor() and for this case this new API is too verbose and too inefficient. I believe 
that with annotations we are seeking for making NetBeans APIs easier to use. In this case that means to have 
@ProjectServiceProvider(service=Class,type="prjname") that we could add to any class that has default or single 
Project argument type constructor. Please rewrite your API to be more user friendly.

Y03 Each module that starts to use the new annotation needs dependency on projectapi > 1.21.
Comment 8 Milos Kleint 2008-11-10 08:10:26 UTC
Y01: what sort of test? do you have examples? I don't see what is there to test.. The processor is a one liner method,
if there are tests for the layer utilities (which I assume there are) I would be just duplicating them.

Y02: I consider these comments valid but not relevant to the current review. The goal of this issue is a 1:1 rewrite of
the current API situation. Your suggestion is a nice future enhancement though. Please file as separate issue.

Y03: will do.
Comment 9 Jaroslav Tulach 2008-11-10 10:00:40 UTC
Re. Y01: Sample test is part of 
http://www.netbeans.org/nonav/issues/showattachment.cgi/72633/action-registration.diff. Jesse, is it true that 
documentation 
(http://bits.netbeans.org/dev/javadoc/org-openide-filesystems/org/openide/filesystems/annotations/package-summary.html) 
contains no info on writing tests? Can you fix that?

Re. Y03: Thanks.

Re. Y02: The value of 1:1 rewrite is so low, even contra productive, that, in my opinion, it does not justify any 
enhancements. My view is clear: either provider simple, effective annotation based API, or rather don't change 
anything. 

PS: I am ready to get outvoted by next three reviewers that simplicity, effectiveness of NetBeans APIs is not goal.
PPS: I can report this as separate P2 defect, but I'd rather resolve it now, then argue about priorities later.
Comment 10 Milos Kleint 2008-11-10 11:25:39 UTC
<rant>
Y02: How come your "vote" is worth 3 people? Where is this "rule" written down?

With such attitude on your side, I'm either rolling back my changes altogether and forget about annotations or commit
them anyway. Not decided yet.

I suspect your "low value" claim is tied to your attempts to push your classloading agenda (involving declarative
projects in lookup) that I rejected earlier. Are we in vendetta mode?

</rant>

Comment 11 Jaroslav Tulach 2008-11-10 12:24:11 UTC
Re. 3 reviewers. I am referring to standard review, which needs four people. I expect to be one of them. In such case 
you need three of them to be against me, or (which I realized just now) you can have just two, if one abstains.

Are you OK with standard review? What are the reviewers of your choice?
Comment 12 Jesse Glick 2008-11-10 17:20:44 UTC
Y01 - The sample patch had a test, but it never passed. For a simple annotation I don't see anything worth testing, it
is pretty straightforward. If you want you can just make an annotated nested class in the test and check that the SPI
works. For more complicated cases it is best to read ServiceProviderProcessorTest for inspiration.


Y03 is incorrect. Using the annotation produces no runtime dependency on any new feature of projectuiapi, so an old dep
on projectuiapi is fine. For a module which has _no_ existing dep on projectuiapi, you can add a dep with
<build-prereq/> and <compile-dep> but no <runtim-dep> (in which case there is no version to specify).


As to Y02, it seems to me that the originally proposed annotation is fine and should be made available, but that we
should introduce a new runtime SPI (and matching annotation) for the cases Yarda identified. (There may be cases that do
not fit into the new SPI, in case the lookup provider conditionally decides whether or not to add items to lookup based
on some aspect of the project.) Issue #150194 is the proper place to discuss such a new SPI, where I have already noted
some problems regarding LookupMerger with a lazier SPI and possible solutions. Yarda's latest suggestion (specifically
reliance on a class with a particular constructor pattern) is not mentioned there yet.
Comment 13 Jaroslav Tulach 2008-11-10 20:48:37 UTC
Re. Y03. Surprising. Sorry.
Re. JG on Y02: It is unnatural to make simple things overly complex. Simple things shall be easy, complex things shall 
be possible. We already have the second part (complex things in complex layer way), let's concentrate on making simple 
things easy. I am not sure if this is what Jesse is saying. Maybe yes. As for myself, I am definitely looking for 
optimal solution to known lookup registration problems. Implementing just
http://www.netbeans.org/nonav/issues/showattachment.cgi/73306/152392.diff
is not sufficient. As I consider implementing halfway, overly complex, inefficient solutions useless, I believe full 
solution with @ProjectServiceProvider or alike is needed.
Re. Y01: Yes, "just make an annotated nested class in the test and check that the SPI works".
Comment 14 Jesse Glick 2008-11-10 21:18:36 UTC
Y01 - see #5bf596a0cd3d.
Comment 15 Quality Engineering 2008-11-12 16:45:34 UTC
Integrated into 'main-golden', will be available in build *200811121401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/f84dd9421ee5
User: Milos Kleint <mkleint@netbeans.org>
Log: #152392 add the LookupProvider.Register annotation api and implementation
Comment 16 Jaroslav Tulach 2008-11-13 08:52:00 UTC
Looks like the code got integrated before the discussions finished. That is slightly unexpected as the last comments 
asked the submitter to find next three reviewers and have a vote. Anyway, let's close this quickly. I suggest the 
reviewers to include me, Jesse, Andrei and Míla. We have one thing to vote about:

It is unnatural to make simple things overly complex. Simple things shall be easy, complex things shall 
be possible. We already have the second part (complex things in complex layer way), let's concentrate on making simple 
things easy. I am not sure if this is what Jesse is saying. Maybe yes. As for myself, I am definitely looking for 
optimal solution to known lookup registration problems. Implementing just
http://www.netbeans.org/nonav/issues/showattachment.cgi/73306/152392.diff
is not sufficient. As I consider implementing halfway, overly complex, inefficient solutions useless, I believe full 
solution with @ProjectServiceProvider or alike is needed.

TCR: Implement easy to use and efficient annotation alá @ProjectServiceProvider. Change the 30-40 usages (~80%) in 
netbeans.org to use it.

Dear reviewers, please decide whether this issue should be closed as "approved with TCR" or "approved" (as in 
f84dd9421ee5) or "rejected". Vote by end of Friday, Nov 14, 2008.

jtulach's vote is "approved with TCR"
Comment 17 Milos Kleint 2008-11-13 08:56:48 UTC
I don't agree with your assesment of the situation. The manjority of people voted by silence as the week passed. That's
how fast track works. 

I integrated the code, that solves this issue, your other comment about @ProjectServiceProvider is unrelated and part of
another issue.
Comment 18 Jaroslav Tulach 2008-11-13 09:50:43 UTC
Let the vote finish.
Comment 19 Milos Kleint 2008-11-13 09:57:53 UTC
fast review has no voting.
Comment 20 Jaroslav Tulach 2008-11-13 10:19:42 UTC
Let the vote finish.

This change does not and did not meet the fast track criteria 
(http://openide.netbeans.org/tutorial/review-steps.html). It was known to be controversial before the integration. 
Also, the integration style did not meet the steps described in fast track section 5 at least in two points (missing 
warning about integration followed by 24h delay). As such actions in section 4 (including switch to standard review) 
are still applicable.

From the discussion so far it seems that agreement between us is going to be hard to reach. That is why I am asking 
for opinion of others. Let the vote finish and let accept its result.
Comment 21 Milos Kleint 2008-11-13 10:43:44 UTC
please don't try to come up with formal reasoning for pushing your agenda and hijacking the issue for your own interest.
The reassignment to myself was a clear indication of the "24 hour" warning period. (Nov 10 07:15:57 +0000 2008) The
integration happened on Nov 11.






Comment 22 Jaroslav Tulach 2008-11-13 12:12:34 UTC
Let the vote finish.
Comment 23 Milos Kleint 2008-11-13 12:28:45 UTC
you didn't have the right to make it a regular review in the first place. You are abusing your own process here.
Citing "If nobody objected against this issue being fast track (by removing API_REVIEW_FAST for a week since submiting,
the summiter shall reassign the issue back to himself and put apireviews@netbeans.org on CC."

you didn't object in time, and your comments were generally unrelated to the review in process anyway.
1. test presence is in no way to relevant to "API" review
2. as was pointed out by jglick, #150194 is the relavant place to discuss your second objection
3. increasing spec version was reslved as wontfix and it's not necessary.

-> there's nothing to vote on.
Comment 24 Jaroslav Tulach 2008-11-13 14:47:25 UTC
If you are right, and "I'm abusing something", then what are you afraid of? Wait for clear say from Jesse, Andrei and 
Míla. They will definitely be against me, in case I am abusing any rules or having insane ideas.

Btw. when talking about abusing: Please note that technically speaking you have not done "reassign the issue back to 
himself and put apireviews@netbeans.org on CC" yet. Search the issue activity to find out.

Comment 25 Milos Kleint 2008-11-13 15:03:02 UTC
please stop spamming everyone.
Comment 26 Jesse Glick 2008-11-13 15:45:42 UTC
This is implemented as designed and should be left closed. The stated intent of this issue was to make it possible to
use annotations to register instances of LookupProvider, a preexisting stable SPI, which is now done.

Whether or not LookupProvider is the ideal SPI for this purpose is an orthogonal issue. Issue #150194 is already open
and discusses the possibility of introducing a new, separate SPI to replace some of its usages. This would most likely
employ a new annotation to make the registration less error-prone and more palatable to developers.

If that tactic can reduce overhead when opening a project in a "big" IDE, then great - this can be checked by
implementing a test patch that maintains exactly the same functionality, opening a j2seproject not using special
features, and measuring (1) change in # of classes loaded (absolute and as percentage), (2) change in average time to
open the project (again abs. & %).

Such an SPI would be applicable to cases where the existing LP impl unconditionally returns a fixed set of instances,
whose constructors can be called in parallel. For other cases, e.g.

ClassPathProviderImpl cppi = new ClassPathProviderImpl(project);
return Lookups.fixed(cppi, new OpenHook(cppi));

or

if (project.getProjectDirectory().getFileObject("hibernate.xml") != null) {
  return Lookups.singleton(new HibernateMapping(project));
} else {
  return Lookup.EMPTY;
}

then LookupProvider likely still needs to be used, unless the code is refactored in more complex ways.
Comment 27 Andrei Badea 2008-11-14 13:13:11 UTC
This has become too political an issue for voting to help, so I am abstaining.

I do, however, think that a ProjectServiceProvider as suggested by jtulach is a better solution from the client's point
of view. It is simpler to use and to understand, and consistent with ServiceProvider. It looks most LookupProvider
usages in NetBeans can be ported to it quite easily. BTW, one that seemingly cannot is in websvc.core's
J2SEWSSupportLookupProvider, which does:

Something s = new Something(project);
SomethingElse se = new SomethingElse(s);

and registers both s and se in the lookup.

It seems there is an agreement that something along the lines of ProjectServiceProvider could be implemented. I do not
think we should have two different annotations serving (almost) the same purpose. PSP should be the preferred solution
for most clients, and those which cannot use it should register a LookupProvider in the layer. (I am assuming that the
introduction of annotations doesn't mean we want to hide the layer concept from the user completely. If not, perhaps
ProjectServiceProvider can be modified to be used on a LookupProvider implementation too.)

Since this issue is so controversial, the solution I would prefer is to rollback the current LookupProvider.Registration
annotation and go back to the drawing board. But this is not a vote. The decision should be up to the module owner.

I would also like to augment jtulach's proposal: it should be possible for the class annotated with
ProjectServiceProvider to also have a constructor taking a single Lookup parameter. This is consistent with
LookupProvider.createAdditionalLookup().
Comment 28 Jaroslav Tulach 2008-11-14 22:39:07 UTC
I have a feeling that nobody could better formulate what solution is in the best interest from the NetBeans project point of view than Andrei. I'd 
like to thank him for his opinion and I am changing my vote to be the same as his. Thanks Andrei and good luck!
Comment 29 Jesse Glick 2008-11-15 17:51:56 UTC
If @ProjectServiceProvider can be implemented as proposed, then let's do it (and please keep all technical comments
about that where they belong, in issue #150194). I don't see how that has any effect on the status of
LookupProvider.Registration: either you decide to deprecate LookupProvider and force all modules to register services
individually, in which case its .Registration (deprecated by association if not explicitly) is irrelevant but harmless;
or you leave it alone (probably mentioning in its Javadoc that many common cases can be handled more directly and
efficiently with PSP), in which case LP.R is useful for those people who still do need to use LP.

"I am assuming that the introduction of annotations doesn't mean we want to hide the layer concept from the user
completely" - we cannot hide the existence of layers under the surface, as people still need to understand the concept
in order to write their own SPIs, debug inevitable problems, perform branding overrides, use issue #26338-based dynamic
layers, etc. But annotations ought be available for every SPI that could use them.