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 getCompletedPositions to ConnectorPageSource #8524

Merged
merged 2 commits into from
Jul 19, 2021

Conversation

raunaqmorarka
Copy link
Member

This allows connectors to report input rows processed
by ConnectorPageSource rather than relying solely on
the positions count of the page returned from getNextPage

@cla-bot cla-bot bot added the cla-signed label Jul 11, 2021
@raunaqmorarka raunaqmorarka requested a review from sopel39 July 11, 2021 20:35
@@ -150,6 +150,21 @@ public void testCustomMetricsScanOnly()
.getMetrics();
}

@Test(timeOut = 30_000)
public void testPhysicalInputPositions()
Copy link
Member

Choose a reason for hiding this comment

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

Could you add similar test for ORC in Hive connector?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's tricky to test, requires hive3 setup for acid and assertions on operator stats outside of memory connector are quite flaky

@sopel39
Copy link
Member

sopel39 commented Jul 12, 2021

lgtm % @martint comment.

I think we should either have explicit method for completed positions OR we should move other methods (like completed bytes) to metrics for consistency. However, I think we can start with former

This allows connectors to report input rows processed
by ConnectorPageSource rather than relying solely on
the positions count of the page returned from getNextPage
@raunaqmorarka
Copy link
Member Author

lgtm % @martint comment.

I think we should either have explicit method for completed positions OR we should move other methods (like completed bytes) to metrics for consistency. However, I think we can start with former

I'm avoiding use of getMetrics as we want to read the new metric in scan operators. If we use that then we would be looking up a hard-coded string like COMPLETED_POSITIONS in the map returned from pageSource.getMetrics. That would be an awkward interface.

@raunaqmorarka raunaqmorarka requested review from sopel39 and martint July 12, 2021 12:00
@sopel39 sopel39 merged commit 07c6e73 into trinodb:master Jul 19, 2021
@sopel39 sopel39 mentioned this pull request Jul 19, 2021
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants