Skip to content
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 RemoteLogTracingEvent to allow tracing the event happen or not #21127

Merged
merged 2 commits into from
Mar 26, 2024
Merged

Add RemoteLogTracingEvent to allow tracing the event happen or not #21127

merged 2 commits into from
Mar 26, 2024

Conversation

chenjian2664
Copy link
Contributor

Description

Add RemoteTracingEvent to support check a database event(in log) happened or not without grab all logs from container at a time.
Update Postgresql testCancellation test to use RemoteTracingEvent.

Additional context and related issues

https://trinodb.slack.com/archives/CP1MUNEUX/p1710496005499909

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Only naming comments.

Also will you remove getRemoteDatabaseEvents in follow-ups?

.ifPresent(remoteDatabaseEvent -> tracingEvents.forEach(tracingEvent -> tracingEvent.accept(remoteDatabaseEvent)));
}

private Optional<RemoteDatabaseEvent> buildEvent(OutputFrame outputFrame)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is getRemoteDatabaseEvents now safe to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple references of the getRemoteDatabaseEvents, I think it's not so easy to remove the method directly.

@hashhar hashhar requested a review from kokosing March 25, 2024 09:19
@chenjian2664
Copy link
Contributor Author

Looks good to me.

Only naming comments.

Also will you remove getRemoteDatabaseEvents in follow-ups?

Thanks for the review.
Yes I am happy to

@chenjian2664 chenjian2664 changed the title Add RemoteLogTracingEvent to allow tracing the event happpen or not Add RemoteLogTracingEvent to allow tracing the event happen or not Mar 25, 2024
@chenjian2664
Copy link
Contributor Author

Error: TestHiveRuntimeAdaptivePartitioningFaultTolerantExecutionJoinQueries.testInnerJoinWithEmptyProbeSide » Timeout testInnerJoinWithEmptyProbeSide() timed out after 120 seconds

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Praveen2112 / @kokosing do you want to take a look before I merge?

@hashhar hashhar merged commit 2572a24 into trinodb:master Mar 26, 2024
62 checks passed
@hashhar hashhar added the no-release-notes This pull request does not require release notes entry label Mar 26, 2024
@github-actions github-actions bot added this to the 444 milestone Mar 26, 2024
@chenjian2664 chenjian2664 deleted the remote_tracing_event branch March 26, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

3 participants