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 query_group for WLM #59

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

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

(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

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.

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.

@@ -1,6 +1,15 @@
CHANGES
=======

1.0.1
Copy link
Member

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.

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

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)


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

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

@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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants