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

Support querying quarry_p database #61

Merged
merged 2 commits into from
Aug 22, 2024
Merged

Support querying quarry_p database #61

merged 2 commits into from
Aug 22, 2024

Conversation

dhinus
Copy link
Member

@dhinus dhinus commented Aug 1, 2024

quarry_p is a new database containing read-only views mirroring 4 tables from Quarry's own database:

  • query
  • query_revision
  • query_run
  • star

The use case for this view is described in T367415: doing stats on query execution times, and on which tables are being queried.

These views should not expose any information that is not already visible from the public Quarry web interface. The "user" and "user_group" table do not have corresponding views, to preserve the privacy of users that have logged in to Quarry but did not create any queries.

Bug: T367415

siddharthvp
siddharthvp previously approved these changes Aug 2, 2024
Copy link
Collaborator

@siddharthvp siddharthvp 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, though I haven't tested.

@dhinus
Copy link
Member Author

dhinus commented Aug 13, 2024

I tried deploying this but trying to make a query against the "quarry" database failed with:

"'Replica' object has no attribute 'database_name'"

I think that's because I missed a required change in the get_host_name function.

@dhinus dhinus force-pushed the T367415 branch 3 times, most recently from 8250d1d to 7f1bb5a Compare August 20, 2024 10:37
quarry_p is a new database containing read-only views mirroring 4 tables
from Quarry's own database:
- query
- query_revision
- query_run
- star

The use case for this view is described in T367415: doing stats on query
execution times, and on which tables are being queried.

These views should not expose any information that is not already
visible from the public Quarry web interface. The "user" and
"user_group" table do not have corresponding views, to preserve the
privacy of users that have logged in to Quarry but did not create any
queries.

Bug: T367415
@dhinus
Copy link
Member Author

dhinus commented Aug 20, 2024

@vivian-rook @siddharthvp I pushed a few fixes and deployed this PR to https://quarry.wmcloud.org/ -- it seems to be working correctly.

Screenshot 2024-08-20 at 14 20 12

@dhinus
Copy link
Member Author

dhinus commented Aug 20, 2024

I just noticed that one node of the k8s cluster is in status "NotReady", but Quarry seems to be working fine.

Update: Both nodes are now "Ready".

@dhinus
Copy link
Member Author

dhinus commented Aug 22, 2024

@vivian-rook do you think this can be merged?

@vivian-rook
Copy link
Contributor

@vivian-rook do you think this can be merged?

If it is working fine while deployed, then yes, it should be merged.

@dhinus
Copy link
Member Author

dhinus commented Aug 22, 2024

Right now it's not working, because pr #65 was deployed in its place, but it was working two days ago. I will merge and deploy from main.

@dhinus dhinus merged commit ecf332a into main Aug 22, 2024
3 checks passed
@dhinus dhinus deleted the T367415 branch August 22, 2024 13:23
@vivian-rook
Copy link
Contributor

I would recommend against deploying off of main in this situation. As it is possible that the changes in the merged PRs or the rebuild of the images have changed things that would impact this PR, even though it was working before. I would recommend deploying off the branch, testing, if it looks good merge it, if it does not deploy off of main to revert the change.

@dhinus
Copy link
Member Author

dhinus commented Aug 22, 2024

I see, thanks. I did double check the other PR #65 and it seemed unlikely to impact this one, but I agree that in general always deploying from the branch is safer (provided that the branch is rebased on top of the current main).

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

Successfully merging this pull request may close these issues.

3 participants