Skip to content

Cache swift authentication token in memcache #4116

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
Merged
rullzer merged 3 commits into from
Mar 29, 2017
Merged

Cache swift authentication token in memcache #4116

rullzer merged 3 commits into from
Mar 29, 2017

Conversation

@icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Mar 28, 2017 •
edited by MorrisJobke
Loading

Prevents having to do an authentication roundtrip every request.

Requires

@mention-bot
Copy link

mention-bot commented Mar 28, 2017

@icewind1991, thanks for your PR! By analyzing the history of the files in this pull request, we identified @butonic, @nickvergessen and @MorrisJobke to be potential reviewers.

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Mar 28, 2017
@icewind1991 icewind1991 added this to the Nextcloud 12.0 milestone Mar 28, 2017
@icewind1991
Copy link
Member Author

icewind1991 commented Mar 28, 2017

22eeff6 is relevant part, the rest is #4112

@icewind1991 icewind1991 changed the title Cache switch authentication token in memcache Cache swift authentication token in memcache Mar 28, 2017
LukasReschke
LukasReschke reviewed Mar 28, 2017
View reviewed changes
lib/private/Files/ObjectStore/Swift.php Outdated
$serviceName = null;
$cachedTokenString = $this->memcache->get('token');
if ($cachedTokenString) {
$cachedToken = unserialize($cachedTokenString);
Copy link
Member

@LukasReschke LukasReschke Mar 28, 2017

Choose a reason for hiding this comment

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

https://github.com/nextcloud/3rdparty/blob/f5555fef8e80d8380efb44dc8b7622a1de573c15/rackspace/php-opencloud/lib/OpenCloud/OpenStack.php#L398-L409 claims this is an array. Can we use json_encode and json_decode thus? :)

Copy link
Member Author

@icewind1991 icewind1991 Mar 28, 2017

Choose a reason for hiding this comment

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

the array contains objects, so we can't json it

@icewind1991 icewind1991 force-pushed the swift-cache-token branch 3 times, most recently from 75c9a30 to 67fe4b2 Compare March 28, 2017 15:15
@MorrisJobke
Copy link
Member

MorrisJobke commented Mar 28, 2017

@icewind1991 Conflicts :/

@icewind1991 icewind1991 force-pushed the swift-cache-token branch 3 times, most recently from ad7a7b5 to e3fe734 Compare March 28, 2017 16:02
@icewind1991
Copy link
Member Author

icewind1991 commented Mar 28, 2017

rebased

@codecov-io
Copy link

codecov-io commented Mar 28, 2017 •
edited
Loading

Codecov Report

Merging #4116 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #4116      +/-   ##
============================================
- Coverage      54.2%   54.18%   -0.03%     
- Complexity    21294    21300       +6     
============================================
  Files          1310     1310              
  Lines         81253    81285      +32     
  Branches       1285     1285              
============================================
+ Hits          44043    44044       +1     
- Misses        37210    37241      +31
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/ObjectStore/Swift.php 0% <0%> (ø) 39 <4> (+6) ⬆️
apps/files_trashbin/lib/Trashbin.php 73.39% <0%> (-0.25%) 133% <0%> (ø)
apps/comments/lib/EventHandler.php 87.5% <0%> (+8.33%) 7% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e26f138...6991b79. Read the comment docs.

MorrisJobke
MorrisJobke approved these changes Mar 28, 2017
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@icewind1991 @MorrisJobke
update rackspace/opencloud to 1.16.0
de83bda 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 @MorrisJobke
cache swift tokens in memcache
205d558 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@MorrisJobke
Copy link
Member

MorrisJobke commented Mar 28, 2017

Updated the merged 3rdparty submodule.

@icewind1991
serialize the token to json instead of using php's serialize
6991b79 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
Copy link
Member Author

icewind1991 commented Mar 28, 2017

@LukasReschke switched to json

@MorrisJobke please retest

MorrisJobke
MorrisJobke approved these changes Mar 28, 2017
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Still works 👍

@rullzer rullzer merged commit 626d03e into master Mar 29, 2017
@rullzer rullzer deleted the swift-cache-token branch March 29, 2017 09:23
@icewind1991 icewind1991 mentioned this pull request Apr 21, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LukasReschke LukasReschke LukasReschke left review comments

@rullzer rullzer rullzer approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

Assignees

No one assigned

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 12.0

Development

Successfully merging this pull request may close these issues.

7 participants

@icewind1991 @mention-bot @MorrisJobke @codecov-io @rullzer @LukasReschke