search index compatibility and other minor changes#63
search index compatibility and other minor changes#63snoopdave merged 4 commits intoapache:masterfrom
Conversation
ameyjadiye
left a comment
There was a problem hiding this comment.
How about replacing custom written cache with better-maintained version within apache commons-collection
OR
a better performer collection?
|
i am not part of the project team, but I will try to respond anyway: LRUCacheImpl is actually very simple. It uses a LinkedHashMap with a single method overridden to make it a fixed size LRU cache. It is synchronized yes, but the critical section is so small that it won't make a difference for a lot if not most deployments. I would like to see it as default tbh (I am using it for my blog). ExpiringLRUCacheImpl: i am not quite sure what problem it was supposed to solve. But the current implementation only removes expired entries after a cache hit. Maybe its just there to refresh the cache if someone edits something in the db without invalidating cached item. But its basically a LRUCacheImple with expiration logic added to it - also fairly simple. If you are running a larger blog server with high traffic you will probably disable rollers cache and have something like nginx running in front of it. A synchronized hash map with get() contention is probably the least of your problems ;) That being said, having a CacheImpl which uses Guava's CacheBuilder would be interesting, but i can't imagine it would be a high priority. I might just give it a try on a rainy day - should be trivial to add, although of limited use for my own blog. |
|
oops, forgot that this pull request was still open. seems like github is automatically adding new commits to PRs. |
|
Sorry I missed this one @mbien. I'll take a look this week. |
|
No worries @snoopdave. The last commit should probably be in a separate PR for spring changes. |
|
Hi @mbien. Thanks for this work. Since this PR updates Spring security, I would like to test it with and LDAP setup to ensure we don't break blogs.apache.org, which uses LDAP. I hope to get to this in the next week or so. |
|
@snoopdave like i mentioned before the spring commit should not be part of this PR, it was my mistake since i didn't expect github to add new commits to existing PRs without asking. I have more spring related changes waiting locally and would put them into a different branch since i agree those would require more testing. Unfortunately github does not seem to provide the option to cherry pick commits from a PR, so the simplest solution would probably be me pushing the branch again with --force without the last commit. This shouldn't break this PR (i hope). |
|
@mbien For the type of changes you're making, you may wish to take a look at my "TightBlog" fork of Roller (https://github.com/gmazza/tightblog), it's only about one-third the size of Roller today so you may find it a much more cleaned-out and solid product to build on. |
|
@gmazza nice I didn't know about tightblog. I would have checked it out this winter before I casually started updating roller if i would have known about it earlier. But most items i wanted to work on are finished now so i am fairly happy where its at (for my needs). Size isn't a big issue, i am running Roller (from my repo) and 3 other JVMs on a 1gb raspi model and it is doing ok. Startup is slow but that is to be expected on that hardware. But good to know that there are alternatives. |
…ion (incompatible/old index). split getFSDirectory(boolean delete) into two methods so that resources can be properly closed.
…en point-release updates. updated most other dependencies to latest versions. tested on 11, 14 and 15b26.
…ed from within synchronized blocks and doesn't escape. added @OverRide.
1b53098 to
c124e3b
Compare
@snoopdave thanks. Let me know if there is anything else to do for me so that this PR can be merged. |
|
Go for it. I think it’s ready
…On Wed, Sep 30, 2020 at 10:50 AM Michael Bien ***@***.***> wrote:
Looks good!
@snoopdave <https://github.com/snoopdave> thanks. Let me know if there is
anything else to do for me so that this PR can be merged.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#63 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIG3VLR5YLWYC7VVR4ZOYLSINAUJANCNFSM4NZEX76A>
.
|
|
@snoopdave perfect - but I have no merge button, all I can do is to comment :)
|
|
well, dang. @mbien I'm not sure why you cannot merge. I will look into that this weekend. Thanks for putting this together. |
|
Hi @mbien, It seems you are still not in roller committers. Have you configured the account with the below setup? Note: You are required to enable 2FA on GitHub before you can gain write-access to repositories |
2FA is always on + i can see myself on this page: tbh i don't really need a merge button since someone with write rights has to review the PRs anyway - i gladly delegate the button pressing to someone else :) |
I set this pull request against 6.0.x since master had bugs and was lacking behind last time I tried (see #58 (comment)).
contents: