You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/12/16 19:48:56 UTC
[GitHub] [geode] demery-pivotal opened a new pull request #7206: geode 9897/client auths tests
demery-pivotal opened a new pull request #7206:
URL: https://github.com/apache/geode/pull/7206
- GEODE-9897: Step 1 Extract SubjectIdGenerator
- GEODE-9897: Step 2 Improve ClientUserAuthsTest
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #7206: GEODE-9897: Improve ClientUserAuthsTest
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7206:
URL: https://github.com/apache/geode/pull/7206#discussion_r771553277
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -97,21 +93,20 @@ public long putSubject(@NotNull Subject subject, long existingUniqueId) {
return newId;
}
- public ClientUserAuths(int clientProxyHashcode) {
- m_seed = clientProxyHashcode;
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ public ClientUserAuths(SubjectIdGenerator idGenerator) {
+ this.idGenerator = idGenerator;
}
- synchronized long getNextID() {
- final long uniqueId = uniqueIdGenerator.nextLong();
- if (uniqueId == m_firstId) {
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ private synchronized long getNextID() {
+ long uniqueId = idGenerator.generateId();
+ if (uniqueId == 0) {
+ uniqueId = idGenerator.generateId();
+ }
+ if (uniqueId == -1) {
// now every user need to reauthenticate as we are short of Ids..
// though possibility of this is rare.
uniqueIdVsUserAuth.clear();
- return m_firstId;
+ return idGenerator.generateId();
Review comment:
The "contract" for `SubjectIdGenerator` is that it returns -1 only if it exhausts all available IDs. So the only way it will return -1 twice in a row only if it cannot generate any IDs in between the -1s.
I put "contract" in scare quotes because it's not written down anywhere. I should probably add a comment on `SubjectIdGenerator` describing the contract.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #7206: GEODE-9897: Improve ClientUserAuthsTest
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #7206:
URL: https://github.com/apache/geode/pull/7206#discussion_r771041033
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -97,21 +93,20 @@ public long putSubject(@NotNull Subject subject, long existingUniqueId) {
return newId;
}
- public ClientUserAuths(int clientProxyHashcode) {
- m_seed = clientProxyHashcode;
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ public ClientUserAuths(SubjectIdGenerator idGenerator) {
+ this.idGenerator = idGenerator;
}
- synchronized long getNextID() {
- final long uniqueId = uniqueIdGenerator.nextLong();
- if (uniqueId == m_firstId) {
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ private synchronized long getNextID() {
+ long uniqueId = idGenerator.generateId();
+ if (uniqueId == 0) {
+ uniqueId = idGenerator.generateId();
+ }
+ if (uniqueId == -1) {
// now every user need to reauthenticate as we are short of Ids..
// though possibility of this is rare.
uniqueIdVsUserAuth.clear();
- return m_firstId;
+ return idGenerator.generateId();
Review comment:
What if `idGenerator.generateId()` return `-1` again? Chances are very slim though.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -97,21 +93,20 @@ public long putSubject(@NotNull Subject subject, long existingUniqueId) {
return newId;
}
- public ClientUserAuths(int clientProxyHashcode) {
- m_seed = clientProxyHashcode;
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ public ClientUserAuths(SubjectIdGenerator idGenerator) {
+ this.idGenerator = idGenerator;
}
- synchronized long getNextID() {
- final long uniqueId = uniqueIdGenerator.nextLong();
- if (uniqueId == m_firstId) {
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ private synchronized long getNextID() {
+ long uniqueId = idGenerator.generateId();
+ if (uniqueId == 0) {
+ uniqueId = idGenerator.generateId();
+ }
+ if (uniqueId == -1) {
Review comment:
If you would, you can refer to `IDS_EXHAUSTED`.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -97,21 +93,20 @@ public long putSubject(@NotNull Subject subject, long existingUniqueId) {
return newId;
}
- public ClientUserAuths(int clientProxyHashcode) {
- m_seed = clientProxyHashcode;
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ public ClientUserAuths(SubjectIdGenerator idGenerator) {
+ this.idGenerator = idGenerator;
}
- synchronized long getNextID() {
- final long uniqueId = uniqueIdGenerator.nextLong();
- if (uniqueId == m_firstId) {
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ private synchronized long getNextID() {
+ long uniqueId = idGenerator.generateId();
+ if (uniqueId == 0) {
+ uniqueId = idGenerator.generateId();
Review comment:
What if `idGenerator.generateId()` return `0` again? Chances are very slim though.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #7206: GEODE-9897: Improve ClientUserAuthsTest
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7206:
URL: https://github.com/apache/geode/pull/7206#discussion_r771593290
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -97,21 +93,20 @@ public long putSubject(@NotNull Subject subject, long existingUniqueId) {
return newId;
}
- public ClientUserAuths(int clientProxyHashcode) {
- m_seed = clientProxyHashcode;
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ public ClientUserAuths(SubjectIdGenerator idGenerator) {
+ this.idGenerator = idGenerator;
}
- synchronized long getNextID() {
- final long uniqueId = uniqueIdGenerator.nextLong();
- if (uniqueId == m_firstId) {
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ private synchronized long getNextID() {
+ long uniqueId = idGenerator.generateId();
+ if (uniqueId == 0) {
+ uniqueId = idGenerator.generateId();
+ }
+ if (uniqueId == -1) {
Review comment:
I think I'm going to leave this as `-1`. The reason is that there's already a weird, subtle coupling between the `-1` here (where it means "IDs exhausted") and the `-1` in `putSubject()` (where it means "please generate an ID").
I say "weird" because, though `-1` has a different meaning in each place, both places must use the same value. That's weird.
If I change this one to `IDS_EXHAUSTED`, that hides the fact that we must use `-1` in both places… which makes it no less weird, but even more subtle.
I'm not happy with the weirdness, the subtlety, or the coupling. But given that there's coupling and it's weird, I'm reluctant make it more subtle.
I considered making `generateId()` return an `OptionalLong`, which would eliminate the need for the generator to treat `-1` as special in any way. I decided not to… out of a perhaps unfounded fear of allocating the `OptionalLong` on the heap. Should I reconsider that?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #7206: GEODE-9897: Improve ClientUserAuthsTest
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7206:
URL: https://github.com/apache/geode/pull/7206#discussion_r771555548
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -97,21 +93,20 @@ public long putSubject(@NotNull Subject subject, long existingUniqueId) {
return newId;
}
- public ClientUserAuths(int clientProxyHashcode) {
- m_seed = clientProxyHashcode;
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ public ClientUserAuths(SubjectIdGenerator idGenerator) {
+ this.idGenerator = idGenerator;
}
- synchronized long getNextID() {
- final long uniqueId = uniqueIdGenerator.nextLong();
- if (uniqueId == m_firstId) {
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ private synchronized long getNextID() {
+ long uniqueId = idGenerator.generateId();
+ if (uniqueId == 0) {
+ uniqueId = idGenerator.generateId();
Review comment:
The contract for an ID generator is to return unique IDs. If it can't do that, it must return -1 to indicate that it has exhausted all unique IDs. So a valid ID generator never returns the same ID twice (0 or otherwise) without first resetting itself and returning -1 to report ID exhaustion.
This contract is currently described only on the implementation of `RandomSubjectIdGenerator`. I will add it to the `SubjectIdGenerator` interface.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on pull request #7206: GEODE-9897: Improve ClientUserAuthsTest
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on pull request #7206:
URL: https://github.com/apache/geode/pull/7206#issuecomment-996999702
Based on @jchen21's comments, I'm going to re-implement the generator to return `OptionalLong` and to use `OptionalLong.empty()` to indicate "no more available unique IDs."
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #7206: GEODE-9897: Improve ClientUserAuthsTest
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #7206:
URL: https://github.com/apache/geode/pull/7206#discussion_r771593290
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientUserAuths.java
##########
@@ -97,21 +93,20 @@ public long putSubject(@NotNull Subject subject, long existingUniqueId) {
return newId;
}
- public ClientUserAuths(int clientProxyHashcode) {
- m_seed = clientProxyHashcode;
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ public ClientUserAuths(SubjectIdGenerator idGenerator) {
+ this.idGenerator = idGenerator;
}
- synchronized long getNextID() {
- final long uniqueId = uniqueIdGenerator.nextLong();
- if (uniqueId == m_firstId) {
- uniqueIdGenerator = new Random(m_seed + System.currentTimeMillis());
- m_firstId = uniqueIdGenerator.nextLong();
+ private synchronized long getNextID() {
+ long uniqueId = idGenerator.generateId();
+ if (uniqueId == 0) {
+ uniqueId = idGenerator.generateId();
+ }
+ if (uniqueId == -1) {
Review comment:
I think I'm going to leave this as `-1` (but see below for an alternative). The reason is that there's already a weird, subtle coupling between the `-1` here (where it means "IDs exhausted") and the `-1` in `putSubject()` (where it means "please generate an ID").
I say "weird" because, though `-1` has a different meaning in each place, both places must use the same value. That's weird.
If I change this one to `IDS_EXHAUSTED`, that hides the fact that we must use `-1` in both places… which makes it no less weird, but even more subtle.
I'm not happy with the weirdness, the subtlety, or the coupling. But given that there's coupling and it's weird, I'm reluctant make it more subtle.
I considered making `generateId()` return an `OptionalLong`, which would eliminate the need for the generator to treat `-1` as special in any way. I decided not to… out of a perhaps unfounded fear of allocating the `OptionalLong` on the heap. Should I reconsider that?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal closed pull request #7206: GEODE-9897: Improve ClientUserAuthsTest
Posted by GitBox <gi...@apache.org>.
demery-pivotal closed pull request #7206:
URL: https://github.com/apache/geode/pull/7206
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org