Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Fix Version/s: 3.2
    • Component/s: Local Write-Read Paths
    • Labels:
      None

      Description

      Add general interface which can intercept per SSTable flush events e.g. - start of the key, columns etc. Make Index interface return such observer on request, which couples index with corresponding SSTable file if needed.

        Activity

        Show
        xedin Pavel Yaskevich added a comment - Patch is available at https://github.com/xedin/sasi/blob/3.x-integration/patches/0001-add-sstable-flush-observer.patch
        Hide
        jasobrown Jason Brown added a comment -

        +1

        Show
        jasobrown Jason Brown added a comment - +1
        Hide
        beobal Sam Tunnicliffe added a comment -

        I guess the following is a prerequisite of https://github.com/xedin/sasi/issues/3, but I'll mention it here anyway:

        SSTableFlushObserver::startRow is misnamed, being called when before a partition, not a row, is written to the SSTable. In fact, rows are not really tracked at all during the flush, only partitions and cells. The corresponding tests also highlight this. So at least one additional method is required on the interface to capture events at this level. Is it possible that RTs may be of interest to some observers too? If so, perhaps we also need another method for those.

        Nit: missing javadoc for Index::getFlushObserver

        The SSTableFlushObserver.Source enum is unused at the moment. Is this in preparation for an upcoming patch, or a leftover from the pre-3.0 version?

        Show
        beobal Sam Tunnicliffe added a comment - I guess the following is a prerequisite of https://github.com/xedin/sasi/issues/3 , but I'll mention it here anyway: SSTableFlushObserver::startRow is misnamed, being called when before a partition, not a row, is written to the SSTable. In fact, rows are not really tracked at all during the flush, only partitions and cells. The corresponding tests also highlight this. So at least one additional method is required on the interface to capture events at this level. Is it possible that RTs may be of interest to some observers too? If so, perhaps we also need another method for those. Nit: missing javadoc for Index::getFlushObserver The SSTableFlushObserver.Source enum is unused at the moment. Is this in preparation for an upcoming patch, or a leftover from the pre-3.0 version?
        Hide
        xedin Pavel Yaskevich added a comment -

        I will rename startRow to startPartition in new terminology, remove Source for now and add getFlushObserver doc.

        Show
        xedin Pavel Yaskevich added a comment - I will rename startRow to startPartition in new terminology, remove Source for now and add getFlushObserver doc.
        Hide
        xedin Pavel Yaskevich added a comment -

        Here is the patch with changes (rebased with trunk).

        Show
        xedin Pavel Yaskevich added a comment - Here is the patch with changes (rebased with trunk).
        Hide
        beobal Sam Tunnicliffe added a comment -

        The v2 patch looks good to me, modulo a couple of nits that can be addressed on commit.

        • The entry in CHANGES.txt is appended to the list for 3.2, whereas lately the convention has been to preprend (just to make subsequent merges easier).
        • The javadoc for SSTableFlushObserver::nextCell refers to the now-renamed startRow.

        Related to the latter point: as I mentioned before, I suspect that when you come to add SASI support for non-compact storage you're going to need some method on the observer to track clusterings within a partition, but I'm fine with waiting until that's actually required before adding that.

        Show
        beobal Sam Tunnicliffe added a comment - The v2 patch looks good to me, modulo a couple of nits that can be addressed on commit. The entry in CHANGES.txt is appended to the list for 3.2, whereas lately the convention has been to preprend (just to make subsequent merges easier). The javadoc for SSTableFlushObserver::nextCell refers to the now-renamed startRow . Related to the latter point: as I mentioned before, I suspect that when you come to add SASI support for non-compact storage you're going to need some method on the observer to track clusterings within a partition, but I'm fine with waiting until that's actually required before adding that.
        Hide
        beobal Sam Tunnicliffe added a comment -

        Pavel Yaskevich seeing as you're not set up for dev builds on http://cassci.datastax.com I've pushed a branch to get a CI run (unfortunately the dtest results are not overly useful at the moment). It may be worth pinging exlt or ptnapoleon in #cassandra-dev to set you up on the CI server.

        branch testall dtest
        10678-trunk testall dtest
        Show
        beobal Sam Tunnicliffe added a comment - Pavel Yaskevich seeing as you're not set up for dev builds on http://cassci.datastax.com I've pushed a branch to get a CI run (unfortunately the dtest results are not overly useful at the moment). It may be worth pinging exlt or ptnapoleon in #cassandra-dev to set you up on the CI server. branch testall dtest 10678-trunk testall dtest
        Hide
        xedin Pavel Yaskevich added a comment -

        Sam Tunnicliffe Thanks, I have fixed two last nits (#1 was good to know ) and pushed to trunk. Will ping the guys you mentioned regarding CI.

        Show
        xedin Pavel Yaskevich added a comment - Sam Tunnicliffe Thanks, I have fixed two last nits (#1 was good to know ) and pushed to trunk. Will ping the guys you mentioned regarding CI.

          People

          • Assignee:
            xedin Pavel Yaskevich
            Reporter:
            xedin Pavel Yaskevich
            Reviewer:
            Sam Tunnicliffe
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development