Uploaded image for project: 'Phoenix'
  1. Phoenix
  2. PHOENIX-4148

COUNT(DISTINCT(...)) should have a memory size limit

    Details

    • Type: Bug
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None

      Description

      I just managed to kill (hang) a region server by issuing a COUNT(DISTINCT(...)) query over a column with very high cardinality (20m in this case).
      This is perhaps not a useful thing to do, but Phoenix should nonetheless not allow to have a server fail because of a query.

      James Taylor, I see there GlobalMemoryManager, but I do not quite see how I'd get a reference to one, once needs a tenant id, etc.

      1. 4148.txt
        9 kB
        Lars Hofhansl

        Activity

        Hide
        jamestaylor James Taylor added a comment - - edited

        See example in GroupedAggregateRegionObserver of GlobalCache.getTenantCache(env, tenantId). You'll want to retrieve the tenantId using ScanUtil.getTenantId(scan). It might be null or not, depending on if the query was done through a tenant-specific connection. Also, this javadoc explains it:

            /**
             * Get the tenant cache associated with the tenantId. If tenantId is not applicable, null may be
             * used in which case a global tenant cache is returned.
             * @param env the HBase configuration
             * @param tenantId the tenant ID or null if not applicable.
             * @return TenantCache
             */
            public static TenantCache getTenantCache(RegionCoprocessorEnvironment env, ImmutableBytesPtr tenantId) {
        
        Show
        jamestaylor James Taylor added a comment - - edited See example in GroupedAggregateRegionObserver of GlobalCache.getTenantCache(env, tenantId) . You'll want to retrieve the tenantId using ScanUtil.getTenantId(scan) . It might be null or not, depending on if the query was done through a tenant-specific connection. Also, this javadoc explains it: /** * Get the tenant cache associated with the tenantId. If tenantId is not applicable, null may be * used in which case a global tenant cache is returned. * @param env the HBase configuration * @param tenantId the tenant ID or null if not applicable. * @ return TenantCache */ public static TenantCache getTenantCache(RegionCoprocessorEnvironment env, ImmutableBytesPtr tenantId) {
        Hide
        lhofhansl Lars Hofhansl added a comment -

        My idea was to reserve a chunk of the HEAP (maybe in increments of 1MB), then allocate stuff onto the hashmap until the chunk is used up, at which point I'd request a new chunk. When finished all chunks would be released. Does that sound about right?

        Show
        lhofhansl Lars Hofhansl added a comment - My idea was to reserve a chunk of the HEAP (maybe in increments of 1MB), then allocate stuff onto the hashmap until the chunk is used up, at which point I'd request a new chunk. When finished all chunks would be released. Does that sound about right?
        Hide
        jamestaylor James Taylor added a comment -

        Yes, that sounds right. The MemoryManager is really just tracking how much memory is used, not doing the actual allocation. So you'd essentially reserve each 1MB chunk and then allocate it yourself (or allocate lots of small chunks that add up to 1MB) - it doesn't have to match up exactly with what you've reserved in the MemoryManager.

        Show
        jamestaylor James Taylor added a comment - Yes, that sounds right. The MemoryManager is really just tracking how much memory is used, not doing the actual allocation. So you'd essentially reserve each 1MB chunk and then allocate it yourself (or allocate lots of small chunks that add up to 1MB) - it doesn't have to match up exactly with what you've reserved in the MemoryManager.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Hmm... How do I find the tenant id all the way down in DistinctValueWithCountServerAggregator?

        Also it seems we're double allocating a bit (unless I am missing something):

        • In GroupedAggregateRegionObserver groupByCache we group by the key
        • DistinctValueWithCountServerAggregator we group again to get the count
        Show
        lhofhansl Lars Hofhansl added a comment - Hmm... How do I find the tenant id all the way down in DistinctValueWithCountServerAggregator? Also it seems we're double allocating a bit (unless I am missing something): In GroupedAggregateRegionObserver groupByCache we group by the key DistinctValueWithCountServerAggregator we group again to get the count
        Hide
        lhofhansl Lars Hofhansl added a comment -

        I have a clumsy patch. (one can check for the size of an aggregator, but getSize() turns out to be very expensive for the count-distinct aggregator. I changed that to be incremental instead. The patch is clumsy (hence not posting it).

        Also I still managed to kill the RS by using up all its heap, so perhaps the default max memory to use for Phoenix is too big.

        I'll continue to look, but it does seem like this needs a bigger discussion. Nothing Phoenix does should ever lead to a RegionServer to exhaust its heap... This would fail the Phoenix query anyway, so it's far better to fail the Phoenix query before that. The memory management in Phoenix is pretty smart, I think, it just seems that it needs to be adjusted.

        Show
        lhofhansl Lars Hofhansl added a comment - I have a clumsy patch. (one can check for the size of an aggregator, but getSize() turns out to be very expensive for the count-distinct aggregator. I changed that to be incremental instead. The patch is clumsy (hence not posting it). Also I still managed to kill the RS by using up all its heap, so perhaps the default max memory to use for Phoenix is too big. I'll continue to look, but it does seem like this needs a bigger discussion. Nothing Phoenix does should ever lead to a RegionServer to exhaust its heap... This would fail the Phoenix query anyway, so it's far better to fail the Phoenix query before that. The memory management in Phoenix is pretty smart, I think, it just seems that it needs to be adjusted.
        Hide
        lhofhansl Lars Hofhansl added a comment - - edited

        Here's the clumsy patch I mentioned. Just handles one the cases for testing.

        DON'T COMMIT.

        I don't like it, at the same time I cannot think of anything better.
        Just parking it here for now.

        Show
        lhofhansl Lars Hofhansl added a comment - - edited Here's the clumsy patch I mentioned. Just handles one the cases for testing. DON'T COMMIT. I don't like it, at the same time I cannot think of anything better. Just parking it here for now.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Hey James Taylor, this is the issue I mentioned. I do not like the patch. If you can think of something better I'd love to know.

        Show
        lhofhansl Lars Hofhansl added a comment - Hey James Taylor , this is the issue I mentioned. I do not like the patch. If you can think of something better I'd love to know.

          People

          • Assignee:
            Unassigned
            Reporter:
            lhofhansl Lars Hofhansl
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development