Skip to content

Add support for Prometheus plugin#15194

Merged
beinan merged 2 commits intoprestodb:masterfrom
fgwang7w:15176
Aug 12, 2021
Merged

Add support for Prometheus plugin#15194
beinan merged 2 commits intoprestodb:masterfrom
fgwang7w:15176

Conversation

@fgwang7w
Copy link
Member

@fgwang7w fgwang7w commented Sep 20, 2020

resolves #15176
Test plan - tesed locally, add UT coverage for this connector

== RELEASE NOTES ==

General Changes
* Add support for Prometheus connector plugin

@aweisberg
Copy link
Contributor

Was there code in this PR copied from another project? If so please follow the attribution guidelines.

@gtangthousandeyes
Copy link

hello team, just curious. Is the prometheus plug-in ready to roll out? I see these in the presto documentation https://prestosql.io/docs/current/connector/prometheus.html but I do not see a prometheus-plugin in the master branch

@findepi
Copy link
Contributor

findepi commented Oct 29, 2020

@gtangthousandeyes PrestoSQL releases are maintained at https://github.com/prestosql/presto.
Also, there is the #troubleshooting channel on Presto Community Slack (https://prestosql.io/slack.html)

@fgwang7w fgwang7w force-pushed the 15176 branch 2 times, most recently from 0ba115c to cd15425 Compare November 3, 2020 03:48
@fgwang7w fgwang7w closed this Nov 8, 2020
@fgwang7w fgwang7w reopened this Nov 8, 2020
@fgwang7w
Copy link
Member Author

fgwang7w commented Nov 8, 2020

@highker could you please help review this pr again? Thanks!

@fgwang7w
Copy link
Member Author

@highker I fixed all the comments and ensure to use TIMESTAMP WITH TIME ZONE from Prometheus connector, please help review and approve the merge as many users are waiting for this feature to go in prestodb. many thanks!

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

will leave it to @zhenxiao to take a look

@fgwang7w
Copy link
Member Author

fgwang7w commented Jun 7, 2021

@qqibrow @highker revision is ready for review. there was some issue with dependency issue and conflicts which are resolved now.

@highker highker removed their request for review June 21, 2021 06:17
@fgwang7w fgwang7w requested a review from highker June 22, 2021 20:23
@fgwang7w
Copy link
Member Author

@zhenxiao could you please help give a final round of review on this PR? Thank you very much !

@fgwang7w fgwang7w force-pushed the 15176 branch 2 times, most recently from c9c6505 to 9d444ef Compare June 23, 2021 03:18
@rohanpednekar rohanpednekar requested a review from aweisberg June 28, 2021 07:07
@fgwang7w fgwang7w requested a review from qqibrow July 2, 2021 07:13
Copy link
Contributor

@qqibrow qqibrow left a comment

Choose a reason for hiding this comment

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

Overall looks good! Left a few comments.

@highker highker removed their request for review July 9, 2021 23:47
@fgwang7w fgwang7w force-pushed the 15176 branch 2 times, most recently from e4cffac to a76a019 Compare July 20, 2021 04:41
@fgwang7w
Copy link
Member Author

fgwang7w commented Aug 8, 2021

@zhenxiao all comments are addressed. please help review this PR, many thanks for your help!

@fgwang7w fgwang7w requested a review from beinan August 9, 2021 16:51
@fgwang7w
Copy link
Member Author

fgwang7w commented Aug 9, 2021

@highker @zhenxiao @qqibrow thank you all for reviewing the PRs. all of your comments were addressed and revisions are made accordingly. Test is good, could we have a final check on this PR? some community folks are asking for the status of this PR waiting for the feature to use. many thanks @beinan for your help too!

@rohanpednekar rohanpednekar requested a review from qqibrow August 10, 2021 18:12
Copy link
Member

@beinan beinan left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me except a couple of coding style issue. But I might hesitate to put this connector into prestodb repo. As we discussed in the TSC meeting, @rongrong do you think if we can put this connector into a separate "plugins" repo? Or we can just merge it into the prestodb repo and then move it out? I think this connector would be a good starting point for the separate "plugins" repo.

Cherry-pick of trinodb/trino#2321

Co-authored by: bentito <btofel@redhat.com>
This commit enables `TIMESTAMP WITH TIME ZONE`, and some code cleanup

Cherry-pick of trinodb/trino#4756

Co-authored by: Dain Sundstrom <dain@iq80.com>
@fgwang7w
Copy link
Member Author

The implementation looks good to me except a couple of coding style issue. But I might hesitate to put this connector into prestodb repo. As we discussed in the TSC meeting, @rongrong do you think if we can put this connector into a separate "plugins" repo? Or we can just merge it into the prestodb repo and then move it out? I think this connector would be a good starting point for the separate "plugins" repo.

Thank you @beinan very much for approving the PR and giving a final review. Since this connector has finally been approved after a long wait and some community members are waiting for this feature to use, will it make sense to merge now?
Separation of plugins into a different repo could be done in a different PR once the decision on how and what connectors to separate from presto is made. It does not make sense to have a bunch of plugin connectors live in one repo and bunch of other connectors in a separate repo. FYI..@tdcmeehan

@tdcmeehan
Copy link
Contributor

tdcmeehan commented Aug 12, 2021

There's some infrastructure that needs to be set up before we can push forward on that approach, such as release jobs. Additionally, though it was discussed at the TSC meeting, it didn't reach consensus, and more followup discussion is needed. I would say for now let's keep it where it is, and we can find another good candidate in the future.

@beinan
Copy link
Member

beinan commented Aug 12, 2021

Thank you @fgwang7w ,@tdcmeehan and all the reviewers of this giant PR! Let me merge this one to unblock @fgwang7w 's further work. I think we can discuss more about when and how we should separate plugins to another repo in the next TSC meeting.

@beinan beinan merged commit 77d7a0c into prestodb:master Aug 12, 2021
@fgwang7w
Copy link
Member Author

Thank you all @zhenxiao @highker @beinan @qqibrow for reviewing this very big PR, and many thanks for all community collaboration and teamwork to make it happen.
@tdcmeehan Thanks for clarification. Happy to help community to build the infrastructure work on presto plugin repo if the final consensus is made given that I added 3 connectors already to the community :-)
@gtangthousandeyes FYI... Prometheus plugin will be available in the upcoming release.

@fgwang7w fgwang7w deleted the 15176 branch August 12, 2021 06:13
@varungajjala varungajjala mentioned this pull request Aug 16, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Roadmap A top level roadmap item

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add support for prometheus connector