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 2020/11/10 12:17:46 UTC
[GitHub] [geode] gaussianrecurrence opened a new pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
gaussianrecurrence opened a new pull request #5725:
URL: https://github.com/apache/geode/pull/5725
- If while calling DistributionImpl.start there was either a
MembershipConfigurationException or MemberStartupException exceptions, its
original cause was not propagated, therefore being unable to properly
tackling issues on startup.
- This commit propagates the cause for both exceptions.
- Also 2 JUint test were added to make sure this is working.
---
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [x] **(N/A)** If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5725:
URL: https://github.com/apache/geode/pull/5725#discussion_r521485764
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionImpl.java
##########
@@ -181,9 +181,9 @@ public void start() {
throw new GemFireSecurityException(e.getMessage(),
e);
} catch (MembershipConfigurationException e) {
- throw new GemFireConfigException(e.getMessage());
+ throw new GemFireConfigException(e.getMessage(), e.getCause());
Review comment:
You should just use the exception here instead of `e.getCause()` otherwise you're discarding one of the stack frames in the exception chain.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionImpl.java
##########
@@ -181,9 +181,9 @@ public void start() {
throw new GemFireSecurityException(e.getMessage(),
e);
} catch (MembershipConfigurationException e) {
- throw new GemFireConfigException(e.getMessage());
+ throw new GemFireConfigException(e.getMessage(), e.getCause());
} catch (MemberStartupException e) {
- throw new SystemConnectException(e.getMessage());
+ throw new SystemConnectException(e.getMessage(), e.getCause());
Review comment:
Ditto
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on pull request #5725:
URL: https://github.com/apache/geode/pull/5725#issuecomment-726110192
Failures are not related to this PR, so it is OK to merge (there is some other issue with `Jetty9CachingClientServerTest`. To re-trigger you can always push an empty commit to your branch: `get commit --allow-empty -m "Trigger CI"`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal merged pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #5725:
URL: https://github.com/apache/geode/pull/5725
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] moleske commented on a change in pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #5725:
URL: https://github.com/apache/geode/pull/5725#discussion_r520744117
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionTest.java
##########
@@ -204,4 +207,30 @@ public void testSendToEmptyListIsRejected() throws Exception {
distribution.send(emptyList, m);
verify(membership, never()).send(any(), any());
}
+
+ @Test
+ public void testExceptionNestedOnStartConfigError() throws Exception {
+ Throwable cause = new RuntimeException("Exception cause");
+ Throwable exception = new MembershipConfigurationException("Test exception", cause);
+ doThrow(exception).when(membership).start();
+ try {
Review comment:
JUnit has [ExpectedException](https://junit.org/junit4/javadoc/4.12/org/junit/rules/ExpectedException.html) which I think works better than `try` `catch` statements. There are some examples in the Geode code base if you want some inspiration
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] gaussianrecurrence commented on pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #5725:
URL: https://github.com/apache/geode/pull/5725#issuecomment-726092069
As it seems concourse is stating Jetty9CachingClientServerTest distributedTest is failing. I've run it multiple times on my local machine and it seems to be actually working. This seems like an sporious failure right? Is there any way in which I can retrigger the concourse execution?
Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5725:
URL: https://github.com/apache/geode/pull/5725#discussion_r520873603
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionTest.java
##########
@@ -204,4 +207,30 @@ public void testSendToEmptyListIsRejected() throws Exception {
distribution.send(emptyList, m);
verify(membership, never()).send(any(), any());
}
+
+ @Test
+ public void testExceptionNestedOnStartConfigError() throws Exception {
+ Throwable cause = new RuntimeException("Exception cause");
+ Throwable exception = new MembershipConfigurationException("Test exception", cause);
+ doThrow(exception).when(membership).start();
+ try {
Review comment:
AssertJ also has a very convenient `assertThatThrownBy()` which lets you assert on the type of exception thrown as well as specific messages, causes, etc. There are numerous examples throughout the test code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5725:
URL: https://github.com/apache/geode/pull/5725#discussion_r521679443
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionImpl.java
##########
@@ -181,9 +181,9 @@ public void start() {
throw new GemFireSecurityException(e.getMessage(),
e);
} catch (MembershipConfigurationException e) {
- throw new GemFireConfigException(e.getMessage());
+ throw new GemFireConfigException(e.getMessage(), e.getCause());
Review comment:
The actual _message_ (string) would be duplicated, but that's OK. You wouldn't be losing part of the full stack trace however. It's not a big deal since you're typically only concerned with the root cause, but it's always better to leave information which may be helpful for debugging.
And, yes your example `throw` is what I'd suggest. That's also what happens just a few lines higher up.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] gaussianrecurrence commented on a change in pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #5725:
URL: https://github.com/apache/geode/pull/5725#discussion_r521490111
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionImpl.java
##########
@@ -181,9 +181,9 @@ public void start() {
throw new GemFireSecurityException(e.getMessage(),
e);
} catch (MembershipConfigurationException e) {
- throw new GemFireConfigException(e.getMessage());
+ throw new GemFireConfigException(e.getMessage(), e.getCause());
Review comment:
Wouldn't the message then be duplicated? I am asking out of ignorance :S
I mean probably it should be something like:
```java
throw new GemFireConfigException("Configuration exception while starting up distribution", e);
```
?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal edited a comment on pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal edited a comment on pull request #5725:
URL: https://github.com/apache/geode/pull/5725#issuecomment-726110192
Failures are not related to this PR, so it is OK to merge (there is some other issue with `Jetty9CachingClientServerTest`. To re-trigger you can always push an empty commit to your branch: `git commit --allow-empty -m "Trigger CI"`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] gaussianrecurrence commented on a change in pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #5725:
URL: https://github.com/apache/geode/pull/5725#discussion_r520747246
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionTest.java
##########
@@ -204,4 +207,30 @@ public void testSendToEmptyListIsRejected() throws Exception {
distribution.send(emptyList, m);
verify(membership, never()).send(any(), any());
}
+
+ @Test
+ public void testExceptionNestedOnStartConfigError() throws Exception {
+ Throwable cause = new RuntimeException("Exception cause");
+ Throwable exception = new MembershipConfigurationException("Test exception", cause);
+ doThrow(exception).when(membership).start();
+ try {
Review comment:
Thanks for the comment! TBH I’ve never used JUnit or mockito before, so any comments are welcome!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] moleske commented on a change in pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #5725:
URL: https://github.com/apache/geode/pull/5725#discussion_r520750337
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionTest.java
##########
@@ -204,4 +207,30 @@ public void testSendToEmptyListIsRejected() throws Exception {
distribution.send(emptyList, m);
verify(membership, never()).send(any(), any());
}
+
+ @Test
+ public void testExceptionNestedOnStartConfigError() throws Exception {
+ Throwable cause = new RuntimeException("Exception cause");
+ Throwable exception = new MembershipConfigurationException("Test exception", cause);
+ doThrow(exception).when(membership).start();
+ try {
Review comment:
Always glad to provide new learning opportunities for folks 😄
Plus no time like the present to learn new things!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] gaussianrecurrence commented on a change in pull request #5725: GEODE-8664: Nest errors in DistributionImpl.start
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #5725:
URL: https://github.com/apache/geode/pull/5725#discussion_r520896345
##########
File path: geode-core/src/test/java/org/apache/geode/distributed/internal/DistributionTest.java
##########
@@ -204,4 +207,30 @@ public void testSendToEmptyListIsRejected() throws Exception {
distribution.send(emptyList, m);
verify(membership, never()).send(any(), any());
}
+
+ @Test
+ public void testExceptionNestedOnStartConfigError() throws Exception {
+ Throwable cause = new RuntimeException("Exception cause");
+ Throwable exception = new MembershipConfigurationException("Test exception", cause);
+ doThrow(exception).when(membership).start();
+ try {
Review comment:
Finally went for AssertJ choice. Now the test is more more clear. Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org