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

HAWQ-1373 - Added feature to reload GUC values using hawq reload-config #1254

Closed
wants to merge 4 commits into from

Conversation

outofmem0ry
Copy link
Contributor

This commit introduces hawq reload-config <object> as a replacement of hawq stop <object> -u|--reload, to reload GUC values without restarting the cluster.

…ig <object> deprecating hawq stop <object> -u
@outofmem0ry
Copy link
Contributor Author

Scoped out some documentation changes which are detailed below -

Files with hawq stop cluster -u
Files with hawq stop cluster --reload
Files with references to reload

@linwen
Copy link
Contributor

linwen commented Jun 14, 2017

Shubham, I think what you've done in this PR is to add a command for hawq, which can reload GUC configs without restarting the system. Currently, this is done by this command "hawq stop cluster --reload", which is a little bit ambiguous in my opinion. So if we all agree on using "hawq reload-config" instead, the old codes related to this reloading GUC logic should be removed. @radarwave @vVineet

@stanlyxiang
Copy link
Member

If we add "hawq reload-config", I think there is no need for "hawq stop cluster -u". 2 commands have same function make me more confused.

@radarwave
Copy link
Contributor

Thanks @outofmem0ry to contribute this, please check below comments.

  1. I think we should keep the legacy command to do reload in case it's hard coded in some user cases, print a deprecated message should be fine.
  2. Make sure all the related help messages and documents are updated.
  3. Chose a proper name for this option before user start to use it, 'reload' or 'reload-config' or something else?
  4. Add test for this option.

@outofmem0ry
Copy link
Contributor Author

@linwen @stanlyxiang @radarwave - Thank you for the comments and suggestions.

@radarwave - Please see the comments inline

  1. I think we should keep the legacy command to do reload in case it's hard coded in some user cases, print a deprecated message should be fine.

The change that is committed prints a message as seen below and exits. I can change the message and let the command continue.

hawq stop cluster -u
To reload GUC values without restarting hawq cluster use 'hawq reload-config <object>'
  1. Make sure all the related help messages and documents are updated.

This is something that will go into incubator-hawq-docs and am working on scoping the change for this.

  1. Chose a proper name for this option before user start to use it, 'reload' or 'reload-config' or something else?

reload sounds good. Also, am open for any other suggestions

  1. Add test for this option.

Sure, let me see how we plug test into the codebase. Any pointers are appreciated.

@outofmem0ry
Copy link
Contributor Author

@radarwave - am in the process of making final changes for this pull request, I don't see any existing test infrastructure for management utilities, am I missing something here. Can you point me to some existing test cases and how they are plugged into hawq code base.

@radarwave
Copy link
Contributor

@outofmem0ry
Please check the cases in below folder as example:
incubator-hawq/src/test/feature/ManagementTool

@outofmem0ry
Copy link
Contributor Author

@radarwave committed the final changes, doc changes pending. Will submit a PR in docs repo.

@radarwave
Copy link
Contributor

Need to fix:
run 'hawq reload' without any other arguments:

hawq reload
Traceback (most recent call last):
File "/usr/local/hawq/bin/hawq", line 215, in
main()
File "/usr/local/hawq/bin/hawq", line 135, in main
sub_args_list.append("-u")
UnboundLocalError: local variable 'sub_args_list' referenced before assignment
Traceback (most recent call last):
File "/usr/local/hawq/bin/hawq", line 215, in
main()
File "/usr/local/hawq/bin/hawq", line 135, in main
sub_args_list.append("-u")
UnboundLocalError: local variable 'sub_args_list' referenced before assignment

tools/bin/hawq Outdated
if len(sub_args.split("-")) > 1:
if "-u" in sub_args or "--reload" in sub_args or "u" in sub_args.split("-")[1]:
deprecationMessage = """
hawq stop <object> -u is being deprecated and replaced by 'hawq reload <object>'
Copy link
Contributor

Choose a reason for hiding this comment

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

This deprecation message is not align with other messages.
Run 'hawq stop cluster -u' then you can see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@radarwave - I deliberately kept the messages printing different so that it is noticeable to the user, as it is a deprecation warning.

/*
Test case for hawq reload <object>. This test changes the value of GUC
log_min_messages to debug. Reloads the cluster and verifies if the change
was reloaded successfully. After the test it resets the valueof GUC to
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo 'valueof'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing it out, will correct it.

@outofmem0ry
Copy link
Contributor Author

@radarwave - Fixed and incorporated above comments.

@radarwave
Copy link
Contributor

LGTM +1

@linwen
Copy link
Contributor

linwen commented Jul 11, 2017

+1

@radarwave
Copy link
Contributor

Thanks for @outofmem0ry 's contribution, I have squashed and merged this commit. Please close this PR.

Welcome to do more contributions.

@outofmem0ry
Copy link
Contributor Author

Thank you @linwen @radarwave . Closing this PR, will also submit a PR for documentation changes to apache/incubator-hawq-docs repo.

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