Skip to content

Support query_group for WLM #59

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support query_group for WLM #59

wants to merge 1 commit into from
+55 −0

Conversation

@Surgo
Copy link
Member

@Surgo Surgo commented Feb 19, 2019

Subject: Set query_group for WLM

Feature or Bugfix

  • Feature
  • Bugfix

Purpose

Detail

  • Run SET query_group TO <specific query group by OPTIONS>

Relates

@Surgo Surgo self-assigned this Feb 19, 2019
@Surgo Surgo requested a review from shimizukawa February 19, 2019 12:59
@shimizukawa
Copy link
Member

shimizukawa commented Feb 19, 2019

(It's my first Draft pull request! If this become a non-draft PR, I should review it.)

@Surgo
Copy link
Member Author

Surgo commented Feb 20, 2019

I'll testing on my application :)

@Surgo
Copy link
Member Author

Surgo commented Feb 20, 2019 •
edited
Loading

Check on my application

  • My settings 'OPTIONS': {'query_group': 'report'}

  • Check queries are labeled to 'report' in svl_qlog

    select query, pid, substring, elapsed, label from svl_qlog where label ='report' order by query;
  query  |  pid  |                          substring                           | elapsed |             label
---------+-------+--------------------------------------------------------------+---------+--------------------------------
 7964883 | 11061 | ************************************************************ | 2189779 | report
 7964910 | 11826 | ************************************************************ |   63912 | report
 7964916 | 11848 | ************************************************************ |   29222 | report
 7964917 | 11847 | ************************************************************ |  128189 | report
 7964926 | 11901 | ************************************************************ |   50180 | report
 7964927 | 11902 | ************************************************************ |   95262 | report
 7964930 | 11907 | ************************************************************ |   56460 | report
 7964931 | 11908 | ************************************************************ | 2937231 | report
 7964932 | 11927 | ************************************************************ | 7731372 | report
 7964933 | 11929 | ************************************************************ | 7613961 | report
(10 rows)

    Raw SQLs (substring) are masked :)

Its works fine for me.

@Surgo Surgo marked this pull request as ready for review February 20, 2019 10:19
@Surgo
Copy link
Member Author

Surgo commented Feb 20, 2019

My application using new query group :)

screenshot 2019-02-20 19 20 33

@shimizukawa This PR is ready fore review. Please review it.

shimizukawa
shimizukawa reviewed Feb 22, 2019
View reviewed changes
Copy link
Member

@shimizukawa shimizukawa left a comment

Choose a reason for hiding this comment

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

Commented.

CHANGES.rst
CHANGES
=======

1.0.1
Copy link
Member

@shimizukawa shimizukawa Feb 19, 2019

Choose a reason for hiding this comment

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

1.0.1 means PATCH version that should be used for minor bug fix.
1.1.0 is good for 'New Features' on semver.

django_redshift_backend/base.py
def init_connection_state(self):
super(DatabaseWrapper, self).init_connection_state()
assignments = []
if self.query_group:
Copy link
Member

@shimizukawa shimizukawa Feb 19, 2019

Choose a reason for hiding this comment

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

I think it would be better that self.query_group is initialized with None on __Init__ method or class variable. (I know init_connection_state always called after get_connection_params)

django_redshift_backend/base.py

if assignments:
with self.cursor() as cursor:
cursor.execute('; '.join(assignments))
Copy link
Member

@shimizukawa shimizukawa Feb 19, 2019

Choose a reason for hiding this comment

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

Why are you using LIST type for assignments variable?
It seems it only have single string object in this method.

@Surgo
Copy link
Member Author

Surgo commented Feb 26, 2019

@shimizukawa Thank you for your review. I'll fix it :)

@Surgo Surgo requested a review from shimizukawa February 26, 2019 10:09
@shimizukawa
Copy link
Member

shimizukawa commented Mar 17, 2019

@Surgo how about this?

@Surgo
Copy link
Member Author

Surgo commented Mar 18, 2019

@shimizukawa I'm sorry but to busy now... I'll fix within weeks..

@shimizukawa
Copy link
Member

shimizukawa commented Mar 18, 2019

OK, no problem. It's just a reminder :)

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

Reviewers

@shimizukawa shimizukawa Awaiting requested review from shimizukawa

Assignees

@Surgo Surgo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Surgo @shimizukawa