New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unprocessed catalog properties to query events #11864

Merged
merged 1 commit into from Nov 7, 2018

Conversation

Projects
None yet
5 participants
@dain
Contributor

dain commented Nov 7, 2018

If a query fails before execution is created, catalog events may not be processed and are not added to the query events.

Addresses issues in #11831

Add unprocessed catalog properties to query events
If a query fails before execution is created, catalog events may not be
processed and are not added to the query events.

@dain dain requested a review from raghavsethi Nov 7, 2018

@dain

This comment has been minimized.

Contributor

dain commented Nov 7, 2018

@hgschmie can you try check if this works for your setup?

for (Entry<String, Map<String, String>> entry : unprocessedCatalogProperties.entrySet()) {
unprocessedCatalogPropertiesBuilder.put(entry.getKey(), ImmutableMap.copyOf(entry.getValue()));
}
this.unprocessedCatalogProperties = unprocessedCatalogPropertiesBuilder.build();

This comment has been minimized.

@findepi

findepi Nov 7, 2018

Member

nit:

this.unprocessedCatalogProperties = unprocessedCatalogProperties.entrySet().stream()
    .collect(toImmutableMap(Entry::getKey, entry -> ImmutableMap.copyOf(entry.getValue()));
@@ -332,14 +333,21 @@ private static QueryIOMetadata getQueryIOMetadata(QueryInfo queryInfo)
private static Map<String, String> mergeSessionAndCatalogProperties(SessionRepresentation session)
{
ImmutableMap.Builder<String, String> mergedProperties = ImmutableMap.builder();
mergedProperties.putAll(session.getSystemProperties());
Map<String, String> mergedProperties = new LinkedHashMap<>(session.getSystemProperties());

This comment has been minimized.

@findepi

findepi Nov 7, 2018

Member

is it Linked for determinism and HashMap to allow overwrites?

This comment has been minimized.

@dain

dain Nov 7, 2018

Contributor

Yep. Internally ImmutableMap is order preserving, and I figured I'd do the same just to make it easier for debugging.

@hgschmie

This comment has been minimized.

Contributor

hgschmie commented Nov 7, 2018

tried the fix locally and it works for me.

@raghavsethi

We probably want to add this to the UI also? Want to play with some Javascript or file me an issue?

@dain dain merged commit 3c15bb1 into prestodb:master Nov 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dain dain deleted the dain:all-session-properties-in-events branch Nov 7, 2018

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