Child pages
  • Best Practices for Kernel Updates
Skip to end of metadata
Go to start of metadata

11-Dec-2008 This work mostly refers to Kernel 1 and is being updated and integrated into Best Practices for High Quality Code by Aaron Zeckoski and Alan Berg.

Information

This documents the best practices for udpates to the kernel (core of framework). Code is currently managed using SVN so many of the practices will revolve around description of that. This will also document some of the higher level development practices that are agreed upon by the technical community.
These practices are important because user needs define the designs that are created by the UX community for Sakai. The UX community defines the tools that Sakai should include. Tool creation by developers brings needed functionality to the overall system. Updates to the framework are important/required to allow improvements to be made which support the tool developers.


Summary

  • Current issues that this addresses
    • Core services do not and cannot evolve with changing tool developer needs
    • Core services often do not meet tool developers needs and can be more of a hindrance than a help
    • Ownership of many kernel services rest on a single developer or is unclear and presents a blockade to updates and high risk when issues occur in the service
    • No clear or well defined processes for kernel updates other than those with commit access can do whatever they want
    • Developers (beginners and veterans) are unsure of who to ask about tool needs and proposed changes to kernel services
    • Unclear who will define a direction for the kernel (core service) and how quality is maintained
    • Critical framework code is untested and therefore unreliable and is often changed without evaluating the impact of the changes
    • Similar to many of the issues that the kernel release addresses
  • Goals (of using these practices)
    • Tool developers have the functionality that is needed or it is added in a timely way
    • Core service, kernel, and framework stability (better for everyone)
    • Better Saaki application performance and ability to handle load (better for admins)
    • More service interface stability and reduction of breaking changes (better for tool developers)
    • Support for large scale improvements/updates in the kernel which are not disruptive (better for developers)
    • Diverse control over the framework to ensure good development practices (better for developers)
  • Benefits
    • A clearly defined team of people to work on and ensure quality in the Sakai kernel
    • Higher quality and reliability for Sakai services (and therefore the product as a whole)
    • Better understanding of how changes to the kernel are made and approved (for veterans and beginners in the community)
    • Definition of best practices and processes around the kernel
    • A well defined community process for working within the most critical parts of the codebase

Best practices for Sakai Kernel updates

Kernel Services (components)

  • The kernel services should be clearly defined and listed. This list should be kept up to date.
  • The kernel services should be considered as the foundation on which Sakai is built and maintained carefully.
  • The kernel services should be seen as supporting the tool developers. It is critical that they are easy to use, helpful, and extendable so that they do not get in the way of the tool development community.

Kernel team

  • A kernel team should be formed which approves and monitors changes to the kernel
    • This team should consist of trusted members of the technical community and at least one junior member (to be mentored)
  • All members should have equal representation in the group
  • Members should have demonstrated ability to understand the kernel, time to work on it (prefereably assigned by their institution), and self-motivation to join the team
  • Ideally members should be chosen by some other group/person and confirmed by the current kernel team
  • Members may leave the group at any time and should be allowed to rejoin
    • The list of active members should be updated as needed
  • Inactive members should be removed after a certain period of inactivity (2 months?).
    • Inactive members may rejoin if they have time to work on the kernel again
  • The team should consist of some minimum number of members (4?) but there will be no maximum unless decision making becomes a problem, if that happens we will deal with it then
  • Kernel team practices and decisions should be documented publically and mentioned on the public lists
    • This should happen well before any mandatory changes are made to the codebase or best practices are enforced

Kernel code changes and updates

  • All kernel development should happen in branches
    • Branches should be created for anyone who asks
      • Branch creation should be timely (ideally within one day)
      • Team branches should be encouraged to reduce conflicts and increase collaborative overview
    • This does not have to apply to simple fixes (like pom/classpath fixes), that would just cause clutter and slow down everyone needlessly
      • The kernel team should be allowed to decide on a case by case basis what fixes are too simple to require a branch
  • Large scale changes which require access to many projects in Sakai should be managed with a branch as well. The large scale changes should be merged and tested outside of the main svn. Once the changes are shown to be safe, they should be merged in with a single commit action by a branch manager
    • An example of this would be a change to an interface which will no longer be supported (i.e. deprecated). This change would require access to many projects to fix the calls to the interface so they are using the supported method
  • Branch merges should be approved by the owners of that project and should include the kernel team
    • Approval should require some percentage of members (70%?) to agree
    • Dissention and reasons should be noted in the svn merge commit message
    • Owners are responsible for responding to a merge request in a resonable amount of time (1 week?)
      • If the owner does not respond within the reasonable amount of time then the merge decision is made by the framework team
  • All branches which are being merged should be mentioned to the community before they are merged (via the dev list probably)
    • This is not a community vote but is more of a way to let people know what is happening and keep a public record
    • Community member objections should be considered before the kernel changes are committed but they do not have to be addressed

Code Change Process Example

  • This is an example of the process (as suggested by the rest of this document) that would allow for higher quality code, control over added features, stability in trunk, and code reviews
  1. Developer(s) publicly mention an idea to the team and community (optional but highly recommended)
    • Discussed ideas are unlikely to be rejected so it is prudent to discuss ideas before working on them
  2. Developer(s) request a code branch from the branch manager to implement/work on their idea (all requests are granted and should be granted in a timely manner)
    • Developer(s) with commit access may create their own branches but should not skip the first step (discussion)
  3. Developer(s) write the code in a branch until they are done
    • They may have to merge in other changes to the code they are working on if merges take place while they are developing, it should not be the responsibility of the branch manager to resolve conflicts, instructions should be provided which explain how the merges are done
  4. Developer(s) announce they want to have their branch merged in (this should be public but it is probably a good idea to copy svn@collab.sakaiproject.org)
  5. Branch manager asks the kernel team to please look at the branch (before it is merged)
  6. Kernel team evaluates the branch (code review) and approves or suggests ways to fix the code
    • This may include rejection of the merge if it was not discussed publicly before work began and/or does not fit the goals of the community
    • The kernel team may help correct the code if they have time and the Developer(s) grant them permission
    • This cycle continues until the branch is acceptable to the team
    • Kernel team may suggest QA testing is required before merging thus it may go to QA at this point
  7. Branch manager receives approval from the team (or QA) and merges the branch into trunk
  • NOTE: Developer defined - anyone including a member of the team or the main committer for kernel code
  • NOTE: JIRA should probably be used to track this process and it might even be worth creating a workflow

Kernel commit access and ownership

  • No one should have global commit access except the Sakai release management team
    • The release managers are a trusted group which uses trusted accounts
    • If they are also developers then they should have a separate account which they use for committing their development code
  • All pieces of the kernel (e.g. user project) should IDEALLY be owned by at least 2 persons representing 2 different institutions
    • This rule can be broken if an inadequate number of people are willing to take ownership but the community should strive to find owners for all pieces of the kernel
  • Branch merges are conducted by the owners or the release manager
    • In most circumstances it is desirable to have the release manager perform the merge
    • It should be verified that the merge will build before the merge is actually committed
  • Global commit access may still be necessary for especially large changes. This should be allowed but should only happen for limited periods (e.g. one month).

Use of existing projects/code

  • Sakai should try to minimize the amount of code which is maintained by the community
    • If an existing stable open source project provides the functionality we need, we should use it

Programming Best Practices

Kernel service interfaces (APIs)

  • Kernel interfaces should have up to date and comprehensive Javadocs.
  • Kernel interfaces which are expected to have local/multiple implementations should be designed to minimize required development work
    • Ideally the number of methods to implement should be minimized and additional capabilities and flexibility should be added in via additional interfaces
    • Logical division is critical and implementers should only need to implement the parts that they need. Interfaces should be split up so that it is easy to only implement what is needed
    • Interfaces which are split up should use a common parent interface so that it is easy to refer to the implemented set of interfaces via the common root
  • Services should be designed in a way that is easy to extend and should be divided into multiple interfaces along logical boundaries.
  • Changes to existing interfaces should require deprecation
    • Deprecated methods should have explanations of the deprecation and should include a tentative date for removal.
    • Deprecated methods should stay in interfaces for as long as possible. Recommend no more than 1 year and no less than 3 months.
    • Deprecated methods should log warnings for methods that are no longer correctly supported
    • Deprecated methods should throw exceptions when they are not functional
  • Interfaces should expose as few dependencies as possible.
    • Interfaces should use IDs instead of full objects where possible
  • Interfaces should include efficient methods to support limited returns of objects, sorting, and paging
  • Naming convention for service interfaces?

Providers

  • Providers should be supported in as many kernel services as possible.
  • Pull type and push type providers should be supplied to make enhancement of Sakai as easy as possible.
  • Providers should adhere to the Service Interfaces guidelines.
  • Service and domain objects (model objects) should be easily extendable
    • Any service or domain object interfaces which clients are expected to use should include a conservative "base" implementation class
      • One way to think about this is as an "abstract" class without the "abstract" attribute.
      • Allows developers focus on the methods which they're actually interested in and give them some protection against interface changes over time.
  • The kernel should support delegation/plugins as much as practical to allow for expansion of functionality without requiring institutions to make local code changes.

Automated Testing

  • All kernel services must include programmatic unit testing and/or integration testing.
  • Tests should ideally be reviewed by at least one other developer
  • Services should ideally include stubs which are accurate representations of how the service would operate.
    • Stubs should use test data which is returned by the stub and located in a separate static class
    • Stubs should be located in the "sakai-mocks" project

Other Best Practices

Terminology

  • Kernel/Framework/Core services - the main services (APIs, IMPLs, UTILs) that make up the Sakai framework (e.g. UserDirectoryService). This does not include any tools or webapps.
  • API/Interface - Java interfaces
  • Kernal/Framework Pieces - A subproject which makes up part of the framework
  • No labels

24 Comments

  1. Here are a couple of quick comments and opinions.

    • Framework
      • The APIs will have up to date and comprehensive Javadocs.
      • The Framework will support delegation / plugins as much as practical to allow for expansion of functionality without requiring framework changes.
      • Bloat will be avoided.
    • Framework Team
      • The reasons for a significant decision will be documented and explained well before any mandatory change is rolled out.

    There should also be some discussion of the philosophy of the framework and API design, so that there is convergence in way that the Framework will be used.

    1. Comments integrated

  2. This is great stuff, Aaron. O'Reilly should turn it into a book.

    I'll restrict direct edits to typos. Here are a couple of possible additions:

    • Framework best practices
      • Don't add work without adding value. Sakai should try to minimize the amount of generic code which needs to be maintained by the higher education community. If an existing stable open source project provides the functionality we need, we should use it.
    • Providers
      • Any service or domain object interfaces which clients are expected to develop should include a conservative "base" implementation class. (One way to think about this is as an "abstract" class without the "abstract" attribute.) This will let developers focus on the methods which they're actually interested in and give them some protection against interface changes over time.
    1. Comments integrated

  3. Some comments on the "kernel team".

    I think it is difficult to put restrictions on the number of people who are considered to be part of this team. in Kernel Release, 32 kernel services are defined. Setting aside my objections to UI being included there, why should we expect a maximum of 7 people to maintain and evolve that much code?

    Rather than putting limitations on the number of people who are kernel committers, perhaps we should be more concerned with with their qualifications. Such members should:

    1. Have made a significant contribution to kernel code, is in the process of doing so, or gives some assurance of doing so.
    2. Demonstrate some experience, expertise, or deep understanding of the technology area they are responsible for. An expert on users and groups is not necessarily qualified to work on memory management.
    3. Commit to working with the rest of the team to coordinate releases, migration, integration, etc.
    4. Be willing to adopt and work towards Sakai programming standards and best practices.

    If the size of the kernel team becomes too large (a problem I suspect we'd love to have), perhaps some kind of two-tier structure might be considered. Frankly, I don't think you'd have to worry about it for some time.

    1. I agree with Mark that a qualifications-based (merit-based) rather than numbers-based solution is preferable. We should limit numbers only if decision-making becomes a problem.

      1. Adjusted to integrate comments

        1. Could you add the qualification points?

  4. A couple of other things. The best practices in Sakai Programming Best Practices seem to apply to the kernel services as well. Selective migration to here seems like a good idea (Synchronized collections, StringBuilder, auto-generated ids, etc.).

    Additionally, Ian make a comment in passing on the DEV list about inner classes. IMO, inner classes are misused in Sakai. If an inner class is declared to be public, it should be a full class in it's own java file. Public inner classes make can make it very difficult to find things and cause problems for development, testing, etc. Inner classes do have valid uses, but they should be private to an implementation.

    1. There are few good uses for inner classes (emulating closure functionality is one good one) but I agree that public inner classes are not a good idea. That said, they are a standard way of keeping someone from getting to the class and I can only assume that was the goal with all the inner public classes. There are other ways to accomplish this though. The second reason I know of for doing this is to get to data in the outer class. I don't think this is great either, but the general idea of using inner classes is mostly ok as long as they are not model objects which are exposed through an interface. That is probably the distinction we should make.

      1. True, Sakai's legacy service code often implements simple domain model objects in inner classes (probably, as Aaron suggests, to "safeguard" the details of the implementation) when real-world use patterns show they should instead be public. (For example, UserDirectoryProvider-style customizations and improvement of the UserDirectoryService would become much more straightforward once we've replaced the complex of User, UserEdit, BaseUserDirectoryService.BaseUserEdit, and so on, with an API-resident BaseUser class.)

        However, it's hard to put this in completely unambiguous language because it's more a matter of making a judgment call based on projected client needs than a matter of following a clear-cut rule. IMO it's the sort of problem better caught by open peer review and collaboration than by a best-practices bullet point. (Unless the bullet point is "Don't blindly follow the example of existing code when writing a new service.")

        1. Note that there are cases of inner classes being used to implement enumerated types. Granted enum wasn't around in earlier versions of Java, but we can fix it now.

          Aaron's points about inner classes make sense to me. I agree that if we move towards code reviews (+1) that the use of inner classes can be judged on it's merits.

  5. > Ideally members should be chosen by some other group/person and confirmed by the current kernel team

    What "other" group would this be? Perhaps if Sakai had a technical governing group, that might be the place, but such a group doesn't really exist yet. (I don't consider the project planning summits at the Sakai conferences to be such a thing, BTW.)

    > Inactive members should be removed after a certain period of inactivity (2 months?).

    We should be a little careful here. What does "inactive" mean? If I am responsible for the FooBar service and it has no bugs or feature improvements on it in more than a year, should I be removed? Code stability shouldn't be the measure of inactivity, since we do want to retain people to maintain even stable code. Inactive might be defined as: no existing areas of responsibility and no contributions for six months (or even a year). Consider that we are moving towards a much longer development cycle.

    > Kernel services should include as few methods as possible when a developer is expected to implement those interfaces. The goal should be 1-2 methods, however, logical division is more important than creating thousands of tiny interfaces.

    While I can see the point of lean APIs, a goal of 1-2 methods is just plain silly. This is a programming style, not an accepted industry practice. Defining the number of methods in an interface leads to breaking things up for the sake of fragmentation. Some objects are complex by definition and will require corresponding methods to manage them.

    > Services should be designed in a way that is easy to extend and should be divided into multiple interfaces along logical boundaries.

    Forcing APIs into multiple, fragmentary interfaces is also a programming style. I don't think it should be a requirement or even a best practice.

    > Interfaces should use IDs instead of full objects where possible

    While I agree with this in principle, there are potential performance ramifications to consider. There are countless places where code fetches an id and immediate gets the object for it. Yeah, caching helps, but if I need the object, why shouldn't I be able to get it in one call? This is not cut and dry, however. Adding calls to fetch objects would greatly increase coupling and dependencies. More discussion is needed.

    1. Don't confuse the concepts of "people with commit access to things" and the kernel team. They are not the same thing. Having commit access does not make someone a kernel team member and being a kernel team member does not mean you get commit access to the kernel services.
      As for the other group, I don't care who that is nor do I want to define it. I just know it is not the kernel team.

      1. Obviously, I am not clear on the distinction between the proposed Kernel Team and Kernel Committers. After all, the document above states:

        > A kernel team should be formed which approves and monitors changes to the kernel

        Being a committer is irrelevant if changes must be approved by the Kernel Team. If the committer is the one responsible for making final decisions about what the code does, then what does the Kernel Team do besides talk and make recommendations on what "might" be done? If, OTH, the Kernel team decides as a group on the directions that the kernel should take down to the code level, why shouldn't the team consist of committers?

        Who really owns the code?

        I agree with Stephen M. that this page should be split into process/people and technical/architecture.

        1. The proposal is that no one should have global commit or even broad commit to the kernel/framework except for branch/release managers and that all work should be done in branches. Therefore, as you say, commit access is irrelevant related to this team since it simply means the you are a branch/release manager or also responsible for a certain part of the kernel.

          The page is split that way with technical/architecture in a heading at the bottom and the remainder of the items above related to process/people.

        2. It seems clear enough to me... the role of the Kernel Team is legislative, and the role of the Kernel Committers is executive. In a wider sense, it is the role of the team to determine the "Will of the Community" and the committers to make sure it is put into practice once it is determined. The committer only makes "final decisions" about the code in the sense of determining whether a particular piece of code or patch meets the critera as already laid down by the kernel team. The committer does not have policy-making power. Most countries work this way, and questions like "who owns the code" don't end up being too problematic (smile) Who is in charge of your country, Mark? (smile)

          1. The main point I was trying to make was that the language of the document wasn't clear on these points, even seeming conflicting. Your description above is more clear to me (perhaps I understand analogy better).

            I suggest that we make changes to make it clear that the kernel team will evaluate changes and determine what needs to be done at a high level, then turn the work over to committers who will make fine-grained decisions towards implementing the policies set by the team.

            1. It is not really the role of the kernel team to dictate what is worked on. It is more their role to ensure what actually goes in is in line with the needs of the community and the goals of Sakai (e.g. code quality, interface design, comment levels, exception handling, etc.). Thus it is critical to have the goals spelled out and the team diverse enough to represent the community. Effectively, code review BEFORE the code reaches the trunk.

              Therefore, it would be more appropriate to reverse what you said and say something like (for framework/kernel changes only):
              developers run ideas by the team and community (optional), developers write code in a branch (developer could be anyone including a member of the team or the main committer), developer announces they want to have their branch merged in, branch manager asks the kernel team to please look at the branch, kernel team approves or suggests ways to fix the code (this may include rejection of the merge if it was not discussed publically before work began), cycle continues until the branch is acceptable, team may suggest QA testing is required before merging thus it may go to QA at this point, branch manager merges the branch into trunk

              Now that I read this, I should probably put something like this in the document to help make things more clear. Thoughts?

              1. 1. > It is not really the role of the kernel team to dictate what is worked on.

                2. > kernel team approves or suggests ways to fix the code (this may include rejection of the merge

                Do you see how these two statements lead to problems? Who gets to make the final decision? BTW, either way makes sense to me, I just want it to be clear.

                Ultimately, I don't think it will matter either way, because I believe that the kernel team will largely consist of the kernel committers and that it will be a relative small group.

                1. > Do you see how these two statements lead to problems?
                  No. Please elaborate.

                  > Who gets to make the final decision?
                  The kernel team has the overall responsibility to account for the quality of the code that goes into the frameworkernel. Therefore, they have final say and are responsible for explaining (publicly) why some code is inappropriate and how to fix it so it is (if that is possible).

                  I want to make sure it is clear here that people with commit access do not get the ability to skip this process. They have to write code in branches and patches like everyone else. Thus, someone having commit access to part/all of the frameworkernel is not a license to do whatever they want (we have that already and it doesn't work).

                  1. The example helps quite a bit, Aaron. It makes it much clearer that a committer is, in fact, free to do what ever they like. However, the code they produce won't be merged into a release branch until it is approved by the kernel team.

    2. "Kernel services should include as few methods as possible when a developer is expected to implement those interfaces. The goal should be 1-2 methods, however, logical division is more important than creating thousands of tiny interfaces."

      I see where you're coming from with this, but it's not yet clearly expressed. How about:

      "Service interfaces which are expected to have multiple implementations or institution-specific customizations should be designed to minimize required development work. This goal might be met by breaking the service into very narrowly targeted interfaces with only a method or two, by providing safe foundational implementations suitable for subclassing, or by some mix of techniques."

      1. Makes sense to me. I attempted to integrate your comments and then added a bit more to make it more clear (I hope).