Fix QueryContext.getAdditionalFailureInfo bug #13120

kewang1024 · 2019-07-24T07:35:19Z

For queryAllocations(a map) that tracks all the Operator and
its consuming memory for per query, it consists of a special key
FORCE_FREE_OPERATION with a negative number as its value
for memory management purpose. When reading queryAllocations,
we should skip key FORCE_FREE_OPERATION.

rongrong · 2019-07-31T00:02:24Z

presto-main/src/main/java/com/facebook/presto/memory/QueryContext.java

@@ -68,6 +68,8 @@
    private final long maxSpill;
    private final SpillSpaceTracker spillSpaceTracker;
    private final Map<TaskId, TaskContext> taskContexts = new ConcurrentHashMap();
+    // Corresponding to AbstractAggregatedMemoryContext.FORCE_FREE_TAG


Please directly use AbstractAggregatedMemoryContext.FORCE_FREE_TAG. It's more error proof that way.

Please directly use AbstractAggregatedMemoryContext.FORCE_FREE_TAG. It's more error proof that way.

They're in different project, so if we want to directly use, we need to add maven dependency. Should we add maven dependency or just use this this way?

AbstractAggregatedMemoryContext.FORCE_FREE_TAG is in presto-memory-context

rongrong · 2019-07-31T00:22:18Z

presto-main/src/main/java/com/facebook/presto/memory/QueryContext.java

@@ -352,6 +354,7 @@ private String getAdditionalFailureInfo(long allocated, long delta)
        }

        String topConsumers = queryAllocations.entrySet().stream()
+                .filter(entry -> !entry.getKey().equals(FORCE_FREE_TAG))


Another way to do this (if it's not proper to import AbstractAggregatedMemoryContext.FORCE_FREE_TAG directly, is to make sure queryAllocations returned by memoryPool.getTaggedMemoryAllocations().get(queryId) do not include this entry.

cc @nezihyigitbasi

Just chat with @nezihyigitbasi and we agreed that we should just filter this entry out in MemoryPool.getTaggedMemoryAllocations.

rongrong · 2019-08-14T20:25:53Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

@@ -351,6 +353,16 @@ private synchronized void updateTaggedMemoryAllocations(QueryId queryId, String
    @VisibleForTesting
    synchronized Map<QueryId, Map<String, Long>> getTaggedMemoryAllocations()


This is getting quite expensive. Let's just change this to getTaggedMemoryAllocations(queryId) instead. If tests are not easy to fix, introduce a separate function instead so this one (with the old implementation) is only used in tests.

rongrong · 2019-08-20T18:54:01Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

+        Map<String, Long> memoryMap = taggedMemoryAllocations.get(targetQueryId)
+                .entrySet().stream()
+                .filter(entry -> !entry.getKey().equals(FORCE_FREE_TAG))
+                .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue()));


.collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue))

rongrong · 2019-08-20T18:54:22Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

+                .entrySet().stream()
+                .filter(entry -> !entry.getKey().equals(FORCE_FREE_TAG))
+                .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue()));
+        return ImmutableMap.copyOf(memoryMap);


No need to make another copy.

rongrong · 2019-08-20T18:54:47Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

+        if (taggedMemoryAllocations.get(targetQueryId) == null) {
+            return null;
+        }
+        Map<String, Long> memoryMap = taggedMemoryAllocations.get(targetQueryId)


return taggedMemoryAllocations.get(...)

rongrong

Sorry that I missed this in last review. One final thing. Thanks!

rongrong · 2019-08-20T21:35:47Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

+    }
+
+    @VisibleForTesting
+    synchronized Map<String, Long> getTaggedMemoryAllocationsByID(QueryId targetQueryId)


Sorry, missed this. You can just name this function getTaggedMemoryAllocations. The signature is obviously suggesting it's "by ID" so it's not necessary.

getTaggedMemoryAllocations

but we have another getTaggedMemoryAllocations which is called in the test, so I decided to preserve that function and created this new function.
how about changing to getTaggedMemoryAllocation (without the s) ?

That's fine. You can just overload the function name.

That's fine. You can just overload the function name.

I see your point, Done and Done.

rongrong · 2019-08-21T20:22:33Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

@@ -351,6 +352,19 @@ private synchronized void updateTaggedMemoryAllocations(QueryId queryId, String
    @VisibleForTesting
    synchronized Map<QueryId, Map<String, Long>> getTaggedMemoryAllocations()
    {
-        return ImmutableMap.copyOf(taggedMemoryAllocations);
+        return taggedMemoryAllocations.keySet().stream()
+                .collect(toImmutableMap(queryId -> queryId, queryId -> getTaggedMemoryAllocations(queryId)));


return taggedMemoryAllocations.keySet().stream() .collect(toImmutableMap(identity(), this::getTaggedMemoryAllocations));

rongrong · 2019-08-21T20:43:30Z

presto-main/src/main/java/com/facebook/presto/memory/MemoryPool.java

@@ -351,6 +353,19 @@ private synchronized void updateTaggedMemoryAllocations(QueryId queryId, String
    @VisibleForTesting
    synchronized Map<QueryId, Map<String, Long>> getTaggedMemoryAllocations()
    {
-        return ImmutableMap.copyOf(taggedMemoryAllocations);
+        return taggedMemoryAllocations.keySet().stream()
+                .collect(toImmutableMap(identity(), queryId -> getTaggedMemoryAllocations(queryId)));


this:: getTaggedMemoryAllocations

For queryAllocations(a map) that tracks all the Operator and its consuming memory for per query, it consists of a special key FORCE_FREE_OPERATION with a negative number as its value for memory management purpose. When reading queryAllocations, we should skip key FORCE_FREE_OPERATION.

facebook-github-bot added the CLA Signed label Jul 24, 2019

kewang1024 requested a review from rongrong July 24, 2019 16:22

rongrong reviewed Jul 31, 2019

View reviewed changes

rongrong requested a review from nezihyigitbasi July 31, 2019 00:23

kewang1024 force-pushed the fix_getAdditionalFailureInfo_bug branch 3 times, most recently from 52a9ac7 to f723ecc Compare August 13, 2019 18:28

nezihyigitbasi mentioned this pull request Aug 14, 2019

"size is negative" error #13211

Closed

rongrong reviewed Aug 14, 2019

View reviewed changes

kewang1024 force-pushed the fix_getAdditionalFailureInfo_bug branch 2 times, most recently from 370f3c2 to 6956e1f Compare August 19, 2019 23:13

kewang1024 requested a review from rongrong August 20, 2019 03:32

rongrong reviewed Aug 20, 2019

View reviewed changes

kewang1024 force-pushed the fix_getAdditionalFailureInfo_bug branch 2 times, most recently from e212dac to 954e7f1 Compare August 20, 2019 21:27

rongrong reviewed Aug 20, 2019

View reviewed changes

kewang1024 force-pushed the fix_getAdditionalFailureInfo_bug branch 3 times, most recently from 166f532 to 466a6d9 Compare August 21, 2019 20:19

rongrong reviewed Aug 21, 2019

View reviewed changes

kewang1024 force-pushed the fix_getAdditionalFailureInfo_bug branch from 466a6d9 to c560900 Compare August 21, 2019 20:28

rongrong reviewed Aug 21, 2019

View reviewed changes

kewang1024 force-pushed the fix_getAdditionalFailureInfo_bug branch from c560900 to c8eb80b Compare August 21, 2019 21:26

rongrong approved these changes Aug 21, 2019

View reviewed changes

rongrong merged commit 1fc8e5b into prestodb:master Aug 22, 2019

neeradsomanchi mentioned this pull request Sep 9, 2019

Release notes for 0.226 #13340

Closed

kewang1024 deleted the fix_getAdditionalFailureInfo_bug branch March 12, 2023 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix QueryContext.getAdditionalFailureInfo bug #13120

Fix QueryContext.getAdditionalFailureInfo bug #13120

kewang1024 commented Jul 24, 2019

rongrong Jul 31, 2019

kewang1024 Jul 31, 2019

kewang1024 Jul 31, 2019

rongrong Jul 31, 2019

rongrong Jul 31, 2019

rongrong Aug 12, 2019

rongrong Aug 14, 2019

rongrong Aug 20, 2019

rongrong Aug 20, 2019

rongrong Aug 20, 2019

rongrong left a comment

rongrong Aug 20, 2019

kewang1024 Aug 20, 2019

rongrong Aug 20, 2019

kewang1024 Aug 21, 2019

rongrong Aug 21, 2019

rongrong Aug 21, 2019

		@@ -351,6 +353,16 @@ private synchronized void updateTaggedMemoryAllocations(QueryId queryId, String
		@VisibleForTesting
		synchronized Map<QueryId, Map<String, Long>> getTaggedMemoryAllocations()

Fix QueryContext.getAdditionalFailureInfo bug #13120

Fix QueryContext.getAdditionalFailureInfo bug #13120

Conversation

kewang1024 commented Jul 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rongrong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment