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

fix inconsistent session value at server and client side #14817

Merged
merged 1 commit into from
Jul 14, 2020
Merged

fix inconsistent session value at server and client side #14817

merged 1 commit into from
Jul 14, 2020

Conversation

weidongduan37
Copy link
Contributor

@weidongduan37 weidongduan37 commented Jul 9, 2020

Fixes #14807

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 9, 2020

CLA Check
The committers are authorized under a signed CLA.

  • ✅ weidongduan37 (0734b0869b6fa15df704eb24bd45a2b2eca6474a)

@mbasmanova
Copy link
Contributor

@weidongduan37 Thanks for the contribution. Would you squash the commits and see if you can update the commit message to follow these guidelines: https://chris.beams.io/posts/git-commit/?

Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

@weidongduan37 this fix looks good, but I'm concerned there's no tests for it.

As you mentioned earlier, there doesn't seem to be a current system property that would ever be URL encoded. Perhaps we can enhance the tests in TestPrestoDriver to add a dummy catalog to the TestingPrestoServer that has a generic session property, we could set this property then run SHOW SESSION to verify the property is correctly encoded and decoded. There's an example of something similar DistributedQueryRunner.

Previously, server only encode session value as http header content in
response, but do not finish decoding and encoding at client, and
decoding at server when receive request.
Complete corresponding decoding and encoding from both server and client
side.
@weidongduan37
Copy link
Contributor Author

weidongduan37 commented Jul 13, 2020

hi @tdcmeehan , I try to add test case as you suggest, but the ci pipeline failed. I check the log, and it seems not related to my changes(presto-cache related). Could you help me to check anything I missed, or what else I can do? Or How can I re-trigger CI pipeline to have a retry without extra commits? thanks.

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.

Session value is not correct, the bahavior of server and client is not consistent
3 participants