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/09/30 23:27:28 UTC
[GitHub] [geode] mhansonp opened a new pull request #6924: GEODE-9645: Don't notify servers of new DataSerializers
mhansonp opened a new pull request #6924:
URL: https://github.com/apache/geode/pull/6924
on the client if in multi user authentication mode.
--
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] kirklund commented on a change in pull request #6924: GEODE-9645: Don't notify servers of new DataSerializers
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6924:
URL: https://github.com/apache/geode/pull/6924#discussion_r720421746
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java
##########
@@ -181,6 +182,11 @@ public void removeListener(EndpointManager.EndpointListener listener) {
this.listener.removeListener(listener);
}
+ @TestOnly
Review comment:
Do we have guidelines on when to use `@TestOnly` versus `@VisibleForTesting`?
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/PoolManagerTest.java
##########
@@ -37,6 +65,112 @@ public void setUp() {
assertThat(poolManager.getMap()).isEmpty();
}
+ // in Mulituser Auth mode
+ @Test
+ public void Test_thatDataSerializerSynchronizationMessagesAreNotSent() {
Review comment:
You should use camel case. The method names should start with lowercase unless there's some unusual reason why it helps with organization and understanding of a class.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PoolManagerImpl.java
##########
@@ -63,6 +64,11 @@ public static PoolManagerImpl getPMI() {
return result;
}
+ @TestOnly
+ public static void setImpl(PoolManagerImpl poolManager) {
+ impl = poolManager;
+ }
+
Review comment:
I think we're going in the wrong direction with some of these TestOnly setters, especially when the constant or variable is annotated with `@MakeNotStatic` and it needs to have `final` removed to make it work.
--
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] mhansonp commented on a change in pull request #6924: GEODE-9645: Don't notify servers of new DataSerializers
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6924:
URL: https://github.com/apache/geode/pull/6924#discussion_r720471919
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java
##########
@@ -181,6 +182,11 @@ public void removeListener(EndpointManager.EndpointListener listener) {
this.listener.removeListener(listener);
}
+ @TestOnly
Review comment:
No, but I don't want these API to be used at all outside of teset, not just to be private.
--
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] mhansonp commented on pull request #6924: GEODE-9645: Don't notify servers of new DataSerializers
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6924:
URL: https://github.com/apache/geode/pull/6924#issuecomment-934583378
> This seems like a potential departure from what the user might expect the behaviour to be. What are the implications of this change in terms of what the user sees happen when they register a DataSerializer? Also, there is no test coverage added as part of this PR, so it should probably be moved to draft mode until it's in a finished state.
The behavior is correct. It was approved as there is no good solution when using multi-user auth. With regards to the testing, the JUnit testing I had proved to impact other seemingly unrelated tests. So I pulled them. The space where this code needs to work is full of global singletons.
--
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] mhansonp commented on a change in pull request #6924: GEODE-9645: Don't notify servers of new DataSerializers
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6924:
URL: https://github.com/apache/geode/pull/6924#discussion_r720472341
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PoolManagerImpl.java
##########
@@ -63,6 +64,11 @@ public static PoolManagerImpl getPMI() {
return result;
}
+ @TestOnly
+ public static void setImpl(PoolManagerImpl poolManager) {
+ impl = poolManager;
+ }
+
Review comment:
I agree, I would love to work with you to make these tests better.
--
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] mhansonp commented on a change in pull request #6924: GEODE-9645: Don't notify servers of new DataSerializers
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6924:
URL: https://github.com/apache/geode/pull/6924#discussion_r720562455
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/EndpointManagerImpl.java
##########
@@ -181,6 +182,11 @@ public void removeListener(EndpointManager.EndpointListener listener) {
this.listener.removeListener(listener);
}
+ @TestOnly
Review comment:
Removed unit tests.
--
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] mhansonp edited a comment on pull request #6924: GEODE-9645: Don't notify servers of new DataSerializers
Posted by GitBox <gi...@apache.org>.
mhansonp edited a comment on pull request #6924:
URL: https://github.com/apache/geode/pull/6924#issuecomment-934583378
> This seems like a potential departure from what the user might expect the behaviour to be. What are the implications of this change in terms of what the user sees happen when they register a DataSerializer? Also, there is no test coverage added as part of this PR, so it should probably be moved to draft mode until it's in a finished state.
The behavior is correct. It was approved as there is no good solution when using multi-user auth. With regards to the testing, the JUnit testing I had proved to impact other seemingly unrelated tests. So I pulled them. The space where this code needs to work is full of global singletons. I agree though, I have put my PR back in draft.
--
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] mhansonp closed pull request #6924: GEODE-9645: Don't notify servers of new DataSerializers
Posted by GitBox <gi...@apache.org>.
mhansonp closed pull request #6924:
URL: https://github.com/apache/geode/pull/6924
--
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