Skip to content

Use an enum for aggregator cache key byte identifiers instead of static values #6823

Closed

Description

@clintropolis

Based on this discussion the idea is to use an enum instead of a pile of static variables for associating aggregators to cache keys in AggregatorUtil, and use a static initializer to ensure that it is impossible to define the same byte identifier twice. CacheKeyBuilder should also be modified to accept the enum instead of the byte value.

Activity

  1. clintropolis commented on Jan 9, 2019

    @clintropolis
    MemberAuthor

    DimFilterUtils and ExtractionCacheHelper are probably other good candidates for this

  2. drcrallen commented on Jan 9, 2019

    @drcrallen
    Contributor

    How does this play into proprietary extensions? For example, we have a lookup extension that has a special cache key generation mechanism. Is the intention that custom extensions use this enum to generate their cache keys?

  3. clintropolis commented on Jan 10, 2019

    @clintropolis
    MemberAuthor

    Good point @drcrallen, I'm not sure how to best handle this. @leventov as the source of this request do you have any ideas?

  4. leventov commented on Jan 11, 2019

    @leventov
    Member

    An extensible enum pattern could be used.

    interface CacheKeyId {
      byte getId();
    }
    
    enum UpstreamCacheKeyId implements CacheKeyId {
      ...
      RESERVED_1((byte) 200),
      ...
      RESERVED_50((byte) 250)
    }

    Extensions are responsible for using ids from the reserved range.

    Another option is UTF-8 style, special byte for ids from private extensions:

    enum CacheKeyId {
      ...
      EXTENSION(-1)
    }

    Then extensions need to use two-byte identifier, the first byte is EXTENSION.id, and then their custom id.

    More approaches are possible

  5. leventov commented on Jan 29, 2019

    @leventov
    Member

    Another class of scattered constants: GROUPBY_QUERY, TIMESERIES_QUERY, TOPN_QUERY.

  6. stale commented on Nov 5, 2019

    @stale

    This issue has been marked as stale due to 280 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If this issue is still relevant, please simply write any comment. Even if closed, you can still revive the issue at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

  7. clintropolis commented on Nov 5, 2019

    @clintropolis
    MemberAuthor

    Still relevant

  8. stale commented on Nov 5, 2019

    @stale

    This issue is no longer marked as stale.

  9. stale commented on Aug 15, 2020

    @stale

    This issue has been marked as stale due to 280 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If this issue is still relevant, please simply write any comment. Even if closed, you can still revive the issue at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

  10. stale commented on Sep 12, 2020

    @stale

    This issue has been closed due to lack of activity. If you think that is incorrect, or the issue requires additional review, you can revive the issue at any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions