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

Reload method and update version #133

Merged
merged 7 commits into from
May 2, 2023
Merged

Reload method and update version #133

merged 7 commits into from
May 2, 2023

Conversation

ChenyuLInx
Copy link
Contributor

No description provided.

@cla-bot cla-bot bot added the cla:yes label May 2, 2023
)
raise RPCException.from_error(dbt_error(exc, logs=_dict_logs(task_logs)))


Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new endpoint, the rest are mostly format

@ChenyuLInx ChenyuLInx changed the title Reload method Reload method and update version May 2, 2023
Copy link

@davidharting davidharting left a comment

Choose a reason for hiding this comment

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

A few comments and questions, but this makes sense and looks good to me!

I reviewed the new endpoint. I also looked at the rest of the diff to ensure that no unintentional changes slipped in. It does indeed look like everything else is formatting.

Comment on lines +263 to +264
os.kill(os.getpid(), signal.SIGHUP)
return os.getpid()
Copy link

@davidharting davidharting May 2, 2023

Choose a reason for hiding this comment

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

Ah! I was a bit surprised by this, but it makes sense.

  • Instead of actually performing a sighup, I was instead expecting this to execute the sighup handler.
    This ensures the most compatibility with existing behavior, though.
  • I am a bit confused on the SigT naming. I'm not sure what that means. I was expecting this class to be named like ReloadSomethingOrOther
  • So we would call this with a pyaload that contains {"method": "reload"}? Easy enough!
  • Will the reload happen synchronously - within band of the api call? Or will a subprocess be started that then runs handle_request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose this because it ensures the most compatibility with existing behavior.

Thanks!! Renamed SigT to Reload in both class and params.

Will the reload happen synchronously:
No, I am not exactly sure whether it is a subprocess or not but this is matches exact behavior of current Sighup signal to rpc

Choose a reason for hiding this comment

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

Awesome thank you so much!

@ChenyuLInx ChenyuLInx merged commit a935d29 into main May 2, 2023
@ChenyuLInx ChenyuLInx deleted the cl/restart_endpoint branch May 2, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants