/presto

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

Remove incorrect getReadTimeNanos() implementations #148

Merged
merged 2 commits into from Feb 5, 2019

Conversation

Reviewers

@sopel39 sopel39

@dain dain

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
4 participants
@findepi @electrum @dain @sopel39
@findepi
Copy link
Member

findepi commented Feb 4, 2019

No description provided.

@cla-bot cla-bot bot added the cla-signed label Feb 4, 2019

@sopel39

sopel39 reviewed Feb 4, 2019
View changes

presto-accumulo/src/main/java/io/prestosql/plugin/accumulo/io/AccumuloRecordCursor.java
@@ -147,7 +145,7 @@ public long getCompletedBytes()
@Override
public long getReadTimeNanos()
{
return nanoStart > 0L ? (nanoEnd == 0 ? System.nanoTime() : nanoEnd) - nanoStart : 0L;

This comment has been minimized.

Sign in to view
Copy link
@sopel39

sopel39 Feb 4, 2019

Member

should we fix that instead?

This comment has been minimized.

Sign in to view
Copy link
@findepi

findepi Feb 4, 2019

Author Member

we could, but i was lazy.

in fact, large majority of the implementations is like that -- they return 0.

This comment has been minimized.

Sign in to view
Copy link
@dain

dain Feb 5, 2019

Member

I don't think this can be implemented for this connector. You really need support from the underlying client to know which calls perform reads.
@electrum

This comment has been minimized.

Sign in to view
Copy link
Member

electrum commented Feb 4, 2019

I think we need to document what this method is supposed to return (like we have for ConnectorPageSource). Then it would be clear that implementations should be recording the time spent to actually read the input.
@dain

dain approved these changes Feb 5, 2019
View changes

@findepi findepi force-pushed the findepi:findepi/master/remove-incorrect-getreadtimenanos-implementations-290831 branch from 9688742 to ace8486 Feb 5, 2019

@findepi

This comment has been minimized.

Sign in to view
Copy link
Member Author

findepi commented Feb 5, 2019 •
edited

@electrum i added io.prestosql.spi.connector.RecordCursor#getReadTimeNanos javadoc:

/**
 * Gets the wall time spent reading data.
 * If read time is not available, this method should return zero.
 *
 * @see ConnectorPageSource#getReadTimeNanos()
 */
long getReadTimeNanos();

@findepi findepi merged commit 1fa2cba into prestosql:master Feb 5, 2019
1 check passed

1 check passed

verification/cla-signed
Details

@findepi findepi deleted the findepi:findepi/master/remove-incorrect-getreadtimenanos-implementations-290831 branch Feb 5, 2019

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