Versioning SPI Review
Submitter: Maros Sandor
Reviewers: Jaroslav Tulach, Radek Matous, Radim Kubacki, Tomas Pavek
Allow external parties to create fully integrated support for arbitrary versioning systems. Make the existing versioning SPI public (used only internally so far).
Approved with TCRs
- Initial review passed (21/3/2007)
- Final review passed (11/4/2007)
- See issue 97278.
- Javadoc can be build from trunk (versioning module, only see package org.netbeans.modules.versioning.spi).
Questions and opinions
See issue 97278. Some of the questions discussed during the review meetings, see notes below.
Notes from the review
Maros: The basic questions at the beginning are: Are we going to do it? Is it worth doing? The API is already done, used internally. All: Yes. Let's make sure it's done correctly when exposed to public.
Maros: Introducing just a basic SPI - minimal, facade over existing non-public APIs (mostly file systems). Typical example: to be able to implement file "move" operation.
All: It's nice it is well defined for particular use cases - versioning - not exposing general FS APIs...
Maros: Maybe it should actually be FS API. The advantage of versioning SPI is that the events are correctly dispatched/delegated to the right target versioning system support.
Jarda: It's a philosophy question... Nobody is going to make this part of public FS API.
Radek: ...and the API and classes are named nicely - expressing the desired way of use.
Jarda: On what modules will the support modules need to depend? We don't want them to use datasystems...
Maros: How to avoid that?
Jarda: FS will set flag when calling versioning module (via the friend API FS provides) and DS will check the flag.
Maros: Problem with file owner/ sharability query - don't work inside FS atomic operations.
Jarda: Also would be great if calling back FS would be avoided... See Y01.
Maros: Should not be a problem - versioning API use File (not FileObject) - should not be needed to call to higher level.
Jarda: Will try to introduce some asserts...
Radim: There's more than just the FS interceptor - this part of the implementing support module should not be linked against FS, DS, etc. But the rest should be... e.g. annotator for creating actions - this part does not require restrictions.
Tomas: So can we agree on we want to avoid callbacks to FS APIs from the module implementing the SPI? (TCR)
Radek: Just advise or enforce?
Jarda: So forcing is not a TCR?
Radek: It's actually a requirement on FS, not versioning SPI. Issue 100582 filed. Will need to check existing vcs plugins before integrating.
Maros: What will be the new module (packages of the current versioncontrol module):
onm.versioning.spi (public package) + onm.versioning (implementation) and onm.versioning.diff (for the diff sidebar)
RM01 Test Compatibility Kit
Maros: What does it mean exactly?
Radek, Jarda: Tests for SPI in versioning module to be used against the versioning support modules. Check it does what is should (as described in javadoc). Can be also used on our existing implementations. Plus documentation on how to run it. This is TCR (At least for the FS interceptor.)
RM02 Poor performance
Radek: Print warnings if something takes too long (calls to interceptors). We are at quite a low level layer and the operations should be fast. Possible problems caused by VS impl are hard to identify. We've already experienced such problems.
Maros: But how to define the limits?
Radek: Can't tell, but warnings don't hurt anybody. We can tune later.
Jarda: Could also be logged in the UI gestures - we could get average numbers.
Maros: What if people will try to call UI handlers?
Radek, Jarda: It's too low level, can't block the thread. Should use something like UserQuestionException...
=> Decision: Time logging is needed - TCR. Consult with performance team (Petr Nejedly) how to do it properly.
Other questions from IZ:
Maros: JG14 Is this contract for getting actions and providing context OK?
Jarda: Let's leave it open for now...
Maros: JG10 OriginalContent.getText vs getOriginalFiles
Maros: After all, managed to eliminate whole OriginalContent class, replaced by one method in VersioningSystem.
Y05,Y06 - Why VCSContext has forNodes? Why public factory methods at all.
Maros: Was my intention as well, but it's not that easy... Will need refactor existing code, but should be able to eliminate it. The SPI clients don't need to create VCSContext object, but I do. So need a public ctor or factory method.
Jarda: Use trampoline. Every reasonable API needs it ;)
Maros: Ok, I'll make it not construable publicly, hide the factory methods ... and use trampoline. (Y06)
Jarda: And use Lookup instead of getNodes() (Y05).
This is a TCR.
Next steps: Make the APIs public (under development), add to the list of NetBeans javadocs, complete use cases in arch doc.
- Avoid callbacks to FS APIs from the module implementing the SPI (interceptor). Issue 100582
- Provide Test Compatibility Kit.
- Implement time logging for delegated low-level file operations (interceptor).
- Eliminate public factory methods in VCSContext, use Lookup getElements() instead of getNodes().
- Add to list of NetBeans javadocs.