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.
Summary: | add annotation based registration of LookupProvider instances | ||
---|---|---|---|
Product: | projects | Reporter: | Milos Kleint <mkleint> |
Component: | Generic Infrastructure | Assignee: | Milos Kleint <mkleint> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | apireviews |
Priority: | P2 | Keywords: | API, API_REVIEW_FAST |
Version: | 6.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | TASK | Exception Reporter: | |
Bug Depends on: | |||
Bug Blocks: | 152984 | ||
Attachments: |
apichange, new annotation
modules rewritten to use annotation |
Description
Milos Kleint
2008-11-05 09:16:47 UTC
Created attachment 73306 [details]
apichange, new annotation
Created attachment 73307 [details]
modules rewritten to use annotation
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 [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. JG01, JG02: both done. thanks for review 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. 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. 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. <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> 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? 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. 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". Y01 - see #5bf596a0cd3d. 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 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" 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. Let the vote finish. fast review has no voting. 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. 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. Let the vote finish. 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. 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. please stop spamming everyone. 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. 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(). 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! 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. |