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.

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

Allow common redis and leveldb connections #12385

Merged
merged 15 commits into from
Sep 27, 2020
Merged

Allow common redis and leveldb connections #12385

merged 15 commits into from
Sep 27, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Jul 30, 2020 •
edited
Loading

Prevents multiple reopening of redis and leveldb connections to the same
place by sharing connections.

Further allows for more configurable redis connection type using the
redisURI and a leveldbURI scheme.

Fix #10121

Signed-off-by: Andrew Thornton art27@cantab.net

@zeripath zeripath added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jul 30, 2020
@zeripath zeripath added this to the 1.13.0 milestone Jul 30, 2020
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 1, 2020
@silverwind
Copy link
Member

The change in CONN_STR seems breaking. Is it?

@zeripath
Copy link
Contributor Author

zeripath commented Aug 1, 2020 •
edited
Loading

No it is not breaking. I wrote all of this to prevent it being breaking: https://github.com/go-gitea/gitea/blob/bf7080a1211b00f0bfb8f6208b6f872eae36275e/modules/nosql/redis.go#L30-L100

@lafriks lafriks added type/enhancement An improvement of existing functionality and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Aug 3, 2020
@zeripath
Allow common redis and leveldb connections
8bc34e4 
Prevents multiple reopening of redis and leveldb connections to the same
place by sharing connections.

Further allows for more configurable redis connection type using the
redisURI and a leveldbURI scheme.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2020 •
edited
Loading

Codecov Report

Merging #12385 into master will decrease coverage by 0.19%.
The diff coverage is 20.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12385      +/-   ##
==========================================
- Coverage   42.84%   42.64%   -0.20%     
==========================================
  Files         662      669       +7     
  Lines       73002    73493     +491     
==========================================
+ Hits        31279    31344      +65     
- Misses      36652    37058     +406     
- Partials     5071     5091      +20
Impacted Files Coverage Δ
modules/cache/cache.go 39.50% <ø> (ø)
modules/cache/cache_redis.go 3.17% <0.00%> (ø)
modules/nosql/manager_redis.go 0.00% <0.00%> (ø)
modules/queue/queue_redis.go 4.76% <0.00%> (+1.12%) ⬆️
modules/queue/unique_queue_redis.go 4.25% <ø> (ø)
modules/session/redis.go 2.15% <0.00%> (ø)
modules/session/virtual.go 60.20% <0.00%> (ø)
modules/nosql/manager_leveldb.go 14.54% <14.54%> (ø)
modules/nosql/manager.go 38.09% <38.09%> (ø)
modules/nosql/redis.go 59.18% <59.18%> (ø)
... and 22 more

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 82179a6...360fd71. Read the comment docs.

zeripath and others added 5 commits September 18, 2020 19:09
@zeripath
Merge remote-tracking branch 'origin/master' into common-nosql
d9c166b
@zeripath
add unit-test
b83cac1 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
as per @lunny
965686f 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
add test
91fd0c5 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@techknowlogick
Merge branch 'master' into common-nosql
360fd71
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 27, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 27, 2020
@lafriks lafriks merged commit 7f8e319 into go-gitea:master Sep 27, 2020
@zeripath zeripath deleted the common-nosql branch September 27, 2020 22:27
@GaetanJuvin
Copy link

I encountered an issue with this PR:

I'm using the Indexer with Redis.

RedisByteFIFOConfiguration is now using ConnectionString (instead of addresses / network / ...)
but when CreateQueue (modules/queue/setting.go)
getQueueSettings is not copying the value of ConnectionString so config is pass with older variables (addresses / network).
Then they are filtered in NewRedisQueue.

So using queue system is going to the default => 127.0.0.1:6379 from ToRedisURI

Or maybe, I miss a step in the config file?

@techknowlogick
Copy link
Member

@GaetanJuvin could you open a new issue about this and include a sanitized copy of your config (secret data switched to FOOBAR)

@GaetanJuvin
Copy link

will do.

zeripath added a commit to zeripath/gitea that referenced this pull request Sep 28, 2020
@zeripath
Fix the issue reported on go-gitea#12385
4136b05 
Missed setting ConnectionString on queuesettings

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Sep 28, 2020
Merged
@zeripath
Copy link
Contributor Author

@GaetanJuvin Thank you for the hint and sorry for the bug - I've put up a PR to fix this oversight.

I'll think again about getQueueSettings it's all a bit too manual and error prone - I think we can leverage MapTo() and consider passing in the ini.

@GaetanJuvin
Copy link

Yep, true. "every line of code is a potential bug" :D
Thanks for #12969 :)

Cheers, thanks for your work!

techknowlogick pushed a commit that referenced this pull request Sep 28, 2020
@zeripath
Fix the issue reported on #12385 (#12969)
5cfc1f5 
Missed setting ConnectionString on queuesettings

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Oct 4, 2020
Merged
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 7, 2020
@zeripath
Avoid setting the CONN_STR in issue indexer queue unless it is meant …
fe55dac 
…to be set

Since the move to common leveldb and common redis the disk queue code (go-gitea#12385)
will check the connection string before defaulting to the DATADIR.

Therefore we should ensure that the connection string is kept empty
unless it is actually set.

Unforunately the issue indexer was missed in go-gitea#13025 this PR fixes this omission

Fix go-gitea#13062

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Oct 7, 2020
Merged
zeripath added a commit that referenced this pull request Oct 7, 2020
@zeripath
Avoid setting the CONN_STR in issue indexer queue unless it is meant …
144fa5a 
…to be set (#13069)

Since the move to common leveldb and common redis the disk queue code (#12385)
will check the connection string before defaulting to the DATADIR.

Therefore we should ensure that the connection string is kept empty
unless it is actually set.

Unforunately the issue indexer was missed in #13025 this PR fixes this omission

Fix #13062

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@6543 6543 6543 left review comments

@silverwind silverwind silverwind left review comments

@lunny lunny lunny left review comments

@lafriks lafriks lafriks approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.

Feature Request: Common Redis/NoSQL connection
9 participants
@zeripath @silverwind @codecov-commenter @GaetanJuvin @techknowlogick @lunny @lafriks @6543 @GiteaBot