You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shiro.apache.org by GitBox <gi...@apache.org> on 2021/06/28 17:44:24 UTC
[GitHub] [shiro] cstamas opened a new pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
cstamas opened a new pull request #308:
URL: https://github.com/apache/shiro/pull/308
The reason is that getMap method return value changed, and
JVM does not find the method (if compiled with HZ3 and runtime
uses HZ4 - same would happen other way around).
Fix has two parts: determines IMap class (v3 or v4),
and them use MethodHandle to invoke it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-870713011
I think someone came up on slack that this could easily be made compatible with both 3 and 4. That's why I asked for a new hz4 module before that came up.
And yes, as we are having a maintenance release here, I'd suggest:
Add hz4 support in main and 1.8 branch
Remove hz3 support from main branch
Check that hz3 support works well in the 1.8 branch.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
bdemers commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-874876976
Could also tweak the OSGI config and run another set of UT's to run with hz4?
Another option would be to create a new module `shiro-hazelcast4` though that gets a little messy as the Shiro v2 version of `shiro-hazelcast` would also be v4.
An alternative to that would be to make the current module `v4` and add a new `shiro-hazelcast3`, but as @cstamas mentioned that might make for a lot of upgrade confusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-875000320
> Another option would be to create a new module `shiro-hazelcast4`
That was my initial idea. Sorry for my brevity, this is what I meant all along.
> though that gets a little messy as the Shiro v2 version of `shiro-hazelcast` would also be v4.
-1. Both Shiro major versions should use `shiro-hazelcast4` for hz4 and stick with `shiro-hazelcast` for hz3. Shiro2 is unreleased. We can change this.
> An alternative to that would be to make the current module `v4` and add a new `shiro-hazelcast3`, but as @cstamas mentioned that might make for a lot of upgrade confusion.
Agreed on this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] cstamas commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-873384953
@bmarwell
> And yes, as we are having a maintenance release here, I'd suggest:
> Add hz4 support in main and 1.8 branch
> Remove hz3 support from main branch
> Check that hz3 support works well in the 1.8 branch.
IMO:
* "maintenance release" should not up major version of a dependency, source code is good as is (this especially if major version of dependency introduces intentional API breakage)
* "add hz4 support" is really just "up the dependency and compile" as then byte code will then work with hz4 (but not with hz3)
* "remove hz3 support from main branch" see above, byte code of this source as-is compiled with hz4 will NOT work with hz3 (just as today compiled against hz3 does NOT work with hz4)
* "check that hz3 support works well in 1.8 branch" it does, I hope tests are in place
Again, IMO here is nothing to be done, but:
* drop this PR
* set HZ version on main branch to hz 4.x
* (maybe) up the HZ version on 1.8 branch to latest hz 3.x
* (maybe) clarify that Shiro 1.x works with hz3 and upcoming Shiro 2.x works with hz4
Personally, I'd not complicate things, by adding burden of "magic" to make something work that was never specd/promised to work. HZ4 did intentional Java package changes, similar to javax.servlet vs jakarta.servlet... so next will be to support those as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-870239523
What is the benefit over an explicit hz4 implementation? This implementation might slow down hz3 a bit. But furthermore, if we replaced this with hz4 at some point in the future, hz3 would intransparently stop working (at least for Shiro 1.8). Thus, might adding a new module be an option?
Otherwise I'd say yes here, too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-870668024
> @bmarwell as I see (in POM), the intent was to seamlessly work with both, HZ 3.x and HZ 4.x (at least that's what import range suggests). But alas, this never worked with both, as AFAIK the `IMap` package change was present in very first 4.0.x release as well. So, at first, it fixes it.
Ah, missed that part. Good thing it is now documented here. 😊
> Second, about "slow down", I am unaware of any code that would call `getCache` at some steady pace/rate. This calls AFAIK happens it majority of cases once in an application (correct me if I'm wrong here). So, while the `getCache` method is "slowed down", nothing else is slow, so unsure what you refer to "might slow down hz3".
Sure, the reflection will only occur once. Nothing to correct here.
Thanks for confirming. LGTM then! 👍🏻
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] cstamas commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-870304309
@bmarwell as I see (in POM), the intent was to seamlessly work with both, HZ 3.x and HZ 4.x (at least that's what import range suggests). But alas, this never worked with both, as AFAIK the `IMap` package change was present in very first 4.0.x release as well. So, at first, it fixes it.
Second, about "slow down", I am unaware of any code that would call `getCache` at some steady pace/rate. This calls AFAIK happens it majority of cases once in an application (correct me if I'm wrong here). So, while the `getCache` method is "slowed down", nothing else is slow, so unsure what you refer to "might slow down hz3".
Finally, as I wrote in JIRA, consider this "hotfix", makes expectation come to reality in Shiro 1.x line. In 2.x line of Shiro I'd drop HZ3 support completely, and would compile (and run against) Hazelcast 4.x only. Drop the Hazelcast 3.x support in short.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] cstamas commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-1008714373
FWIW, I just dropped shiro-hazelcast as dependency from my project: if you consider it, that _one class dependency_ is really an overkill, is not worth it. Hence, am building my CacheManager (is in my sources), so the project does not suffer from this issue. Really, dependency carrying one single class is an overkill IMHO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] jherkel commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
jherkel commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-870357566
Shouldn't the import for OSGI be modified as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
bmarwell commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-870239523
What is the benefit over an explicit hz4 implementation? This implementation might slow down hz3 a bit. But furthermore, if we replaced this with hz4 at some point in the future, hz3 would intransparently stop working (at least for Shiro 1.8). Thus, might adding a new module be an option?
Otherwise I'd say yes here, too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] cstamas edited a comment on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
cstamas edited a comment on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-873384953
@bmarwell
> And yes, as we are having a maintenance release here, I'd suggest:
> Add hz4 support in main and 1.8 branch
> Remove hz3 support from main branch
> Check that hz3 support works well in the 1.8 branch.
IMO (here am talking about current main branch, not this PR changes):
* "maintenance release" should not up major version of a dependency, source code is good as is (this especially if major version of dependency introduces intentional API breakage)
* "add hz4 support" is really just "up the dependency and compile" as then byte code will then work with hz4 (but not with hz3)
* "remove hz3 support from main branch" see above, byte code of this source as-is compiled with hz4 will NOT work with hz3 (just as today compiled against hz3 does NOT work with hz4)
* "check that hz3 support works well in 1.8 branch" it does, I hope tests are in place
Again, IMO here is nothing to be done, but:
* drop this PR
* set HZ version on main branch to hz 4.x
* (maybe) up the HZ version on 1.8 branch to latest hz 3.x
* (maybe) clarify that Shiro 1.x works with hz3 and upcoming Shiro 2.x works with hz4
Personally, I'd not complicate things, by adding burden of "magic" to make something work that was never specd/promised to work. HZ4 did intentional Java package changes, similar to javax.servlet vs jakarta.servlet... so next will be to support those as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] cstamas commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-870709868
> Shouldn't the import for OSGI be modified as well?
@jherkel hm, now I realize I am wrong, it is currently `<hazelcast.osgi.importRange>[3, 4)</hazelcast.osgi.importRange>` and see [1] (sorry, am not OSGi guy).
So maybe my whole premise ("intent was to work with HZ 3 AND HZ4") is completely wrong? As it may also mean that SHIRO-816 is a "non issue" actually (should be closed as such: "Shiro 1.x Hazelcast support supports HZ3 only"), given the module _was never intended_ to work with HZ4 in the first place?
As then, IMO proper solution is to
* drop this PR, close out issue SHIRO-816 ("not an issue: 1.x supports HZ3 only")
* Shiro 1.x is fine as is (will not work with HZ4, works with HZ3)
* in shiro to-be-2.0 introduce support for Hazelcast 4.x, drop support for Hazelcast 3.x (could be this very same module but built against Hazelcast 4)
* done :smile:
[1] https://www.eclipse.org/virgo/documentation/virgo-documentation-3.7.0.M01/docs/virgo-user-guide/html/ch02s02.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] cstamas commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-870304309
@bmarwell as I see (in POM), the intent was to seamlessly work with both, HZ 3.x and HZ 4.x (at least that's what import range suggests). But alas, this never worked with both, as AFAIK the `IMap` package change was present in very first 4.0.x release as well. So, at first, it fixes it.
Second, about "slow down", I am unaware of any code that would call `getCache` at some steady pace/rate. This calls AFAIK happens it majority of cases once in an application (correct me if I'm wrong here). So, while the `getCache` method is "slowed down", nothing else is slow, so unsure what you refer to "might slow down hz3".
Finally, as I wrote in JIRA, consider this "hotfix", makes expectation come to reality in Shiro 1.x line. In 2.x line of Shiro I'd drop HZ3 support completely, and would compile (and run against) Hazelcast 4.x only. Drop the Hazelcast 3.x support in short.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] jherkel commented on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
jherkel commented on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-870357566
Shouldn't the import for OSGI be modified as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] cstamas edited a comment on pull request #308: [SHIRO-816] Hazelcast support does not support HZ v4
Posted by GitBox <gi...@apache.org>.
cstamas edited a comment on pull request #308:
URL: https://github.com/apache/shiro/pull/308#issuecomment-873384953
@bmarwell
> And yes, as we are having a maintenance release here, I'd suggest:
> Add hz4 support in main and 1.8 branch
> Remove hz3 support from main branch
> Check that hz3 support works well in the 1.8 branch.
IMO (here am talking about current main branch, not this PR changes):
* "maintenance release" should not up major version of a dependency, source code is good as is (this especially if major version of dependency introduces intentional API breakage)
* "add hz4 support" is really just "up the dependency and compile" as then byte code will then work with hz4 (but not with hz3)
* "remove hz3 support from main branch" see above, byte code of this source as-is compiled with hz4 will NOT work with hz3 (just as today compiled against hz3 does NOT work with hz4)
* "check that hz3 support works well in 1.8 branch" it does, I hope tests are in place
Again, IMO here is nothing to be done, but:
* drop this PR
* set HZ version on main branch to hz 4.x
* (maybe) up the HZ version on 1.8 branch to latest hz 3.x
* (maybe) clarify that Shiro 1.x works with hz3 and upcoming Shiro 2.x works with hz4
Personally, I'd not complicate things, by adding burden of "magic" to make something work that was never specd/promised to work. HZ4 did intentional Java package changes, similar to javax.servlet vs jakarta.servlet... so next will be to support those transparently as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org