Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.5
    • Labels:
      None

      Description

      Right now, Jackrabbit reports TimeSeries for things like BUNDLE_READ_COUNTER, BUNDLE_WRITE_COUNTER, etc. but there is no way to extend Jackrabbit and report TimeSeries for additional properties. That's because the type of TimeSeries are defined in RepositoryStatistics class as Type enum. Enums in Java cannot be extended which limits to TimeSeries to the Types defined in RepositoryStatistics.

      I suggest that RepositoryStatistics is improved to allow additional TimeSeries. One approach is to define an additional RepositoryStatistics#getType(String) method.

      1. JCR-3243-4.patch
        9 kB
        Mete Atamel
      2. JCR-3243-3.patch
        6 kB
        Mete Atamel
      3. JCR-3243-2.patch
        6 kB
        Mete Atamel
      4. JCR-3243.patch
        6 kB
        Mete Atamel

        Activity

        Hide
        Marcel Reutegger added a comment -

        Latest patch looks good.

        Applied it in revision 1299655.

        I also added a TestAll suite to include it the test run.

        Show
        Marcel Reutegger added a comment - Latest patch looks good. Applied it in revision 1299655. I also added a TestAll suite to include it the test run.
        Hide
        Jukka Zitting added a comment -

        +1 Looks good to me.

        Regarding "RepositoryStatisticsImpl implements Iterable"; since that's only in jackrabbit-core, I'd consider it an implementation detail that we can change without problems if there's a reason to do so. Only the interfaces in jackrabbit-api need to remain strictly backwards-compatible.

        Show
        Jukka Zitting added a comment - +1 Looks good to me. Regarding "RepositoryStatisticsImpl implements Iterable"; since that's only in jackrabbit-core, I'd consider it an implementation detail that we can change without problems if there's a reason to do so. Only the interfaces in jackrabbit-api need to remain strictly backwards-compatible.
        Hide
        Mete Atamel added a comment -

        Can I have someone review and apply this patch please?

        Show
        Mete Atamel added a comment - Can I have someone review and apply this patch please?
        Hide
        Mete Atamel added a comment -

        The final patch that includes suggestions from Alex and a JUnit test for the iterator.

        Show
        Mete Atamel added a comment - The final patch that includes suggestions from Alex and a JUnit test for the iterator.
        Hide
        Alex Parvulescu added a comment -

        > So, I'm not sure which one is worse: return some of the values from the iterator or change a public method.
        I'd definitely choose correctness

        In my view if this patch goes into jr, it should not introduce unexpected behavior.

        Show
        Alex Parvulescu added a comment - > So, I'm not sure which one is worse: return some of the values from the iterator or change a public method. I'd definitely choose correctness In my view if this patch goes into jr, it should not introduce unexpected behavior.
        Hide
        Mete Atamel added a comment -

        Thanks for the feedback. Regarding getType method, yes, definitely, it makes more sense as a static method on Type enum. Regarding the iterator though, I'm not sure how to solve that, that's why I kind of left it the way it is. The problem is that the returned iterator has the following signature: "Entry<Type, TimeSeries>". As you know, with the changes, now we can have String as type and I cannot return those String based types from this iterator. I can change the iterator's signature to "Entry<String, TimeSeries>" and that would enable me to return all the entries but then I'd be changing a public method. So, I'm not sure which one is worse: return some of the values from the iterator or change a public method.

        Show
        Mete Atamel added a comment - Thanks for the feedback. Regarding getType method, yes, definitely, it makes more sense as a static method on Type enum. Regarding the iterator though, I'm not sure how to solve that, that's why I kind of left it the way it is. The problem is that the returned iterator has the following signature: "Entry<Type, TimeSeries>". As you know, with the changes, now we can have String as type and I cannot return those String based types from this iterator. I can change the iterator's signature to "Entry<String, TimeSeries>" and that would enable me to return all the entries but then I'd be changing a public method. So, I'm not sure which one is worse: return some of the values from the iterator or change a public method.
        Hide
        Alex Parvulescu added a comment -

        Good work Mete!

        There is still something I believe the patch did not address yet: RepositoryStatisticsImpl implements Iterable.
        This contract implies that if I call #iterator() I'll get back all the existing entries.
        I would expect that if I define a custom entry, it should still be returned in the #iterator() call, otherwise some consumers of the api will not be able to see all the existing TimeSeries.

        Another minor issue is a method in the RepositoryStatisticsImpl: private Type getType(String type)
        I think this would be better located in the Type enum, as RepositoryStatisticsImpl doesn't really need to deal with identifying a Type by its name.

        I find it interesting that there are no unit tests in the patch. One test case could very well be the #iterator() problem described earlier.

        Show
        Alex Parvulescu added a comment - Good work Mete! There is still something I believe the patch did not address yet: RepositoryStatisticsImpl implements Iterable. This contract implies that if I call #iterator() I'll get back all the existing entries. I would expect that if I define a custom entry, it should still be returned in the #iterator() call, otherwise some consumers of the api will not be able to see all the existing TimeSeries. Another minor issue is a method in the RepositoryStatisticsImpl: private Type getType(String type) I think this would be better located in the Type enum, as RepositoryStatisticsImpl doesn't really need to deal with identifying a Type by its name. I find it interesting that there are no unit tests in the patch. One test case could very well be the #iterator() problem described earlier.
        Hide
        Mete Atamel added a comment -

        One last minor tweak to the patch.

        Show
        Mete Atamel added a comment - One last minor tweak to the patch.
        Hide
        Mete Atamel added a comment -

        A slightly improved version of the patch

        Show
        Mete Atamel added a comment - A slightly improved version of the patch
        Hide
        Mete Atamel added a comment -

        Patch attached.

        Show
        Mete Atamel added a comment - Patch attached.
        Hide
        Mete Atamel added a comment -

        Here's the proposed fix with the least amount of changes I could come up with. I basically tried to eliminate Type enum and use String instead whenever I could.

        Show
        Mete Atamel added a comment - Here's the proposed fix with the least amount of changes I could come up with. I basically tried to eliminate Type enum and use String instead whenever I could.

          People

          • Assignee:
            Unassigned
            Reporter:
            Mete Atamel
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development