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/03/11 13:13:30 UTC
[GitHub] [geode] alb3rtobr opened a new pull request #6116: Test GEODE-8877
alb3rtobr opened a new pull request #6116:
URL: https://github.com/apache/geode/pull/6116
----------------------------------------------------------------
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] alb3rtobr commented on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-806169722
> Please see 2 recommended re-writes in the doc file geode-docs/tools_modules/gfsh/command-pages/start.html.md.erb.
Done, thanks Dave!
--
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] alb3rtobr edited a comment on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
alb3rtobr edited a comment on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-828837263
I have moved two of the code improvements not related with the feature to separated tickets and PRs. I hope this will make the review of this PR easier.
--
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] alb3rtobr commented on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-824633211
> I've read the ticket and made a first pass over the diffs, but I don't understand why StartLocatorCommandWorkingDirectoryTest and StartServerCommandWorkingDirectoryTest need to be deleted in order to support configurable UDP membership address?
It is not needed. Its a code improvement I identified when implementing the functionality. Notice what I wrote on the PR description:
> I have not squashed the commits of the PR because I think it can be useful for reviewers. The two first commits contains the functionality I mentioned previously, while the three next commits contains some code improvements identified after the review of my first PR for this ticket.
Long story: I observed that adding a new parameter in `start locator` or `start server` commands had an impact on all tests of `StartLocatorCommandWorkingDirectoryTest` and `StartLocatorCommandWorkingDirectoryTest`. Those tests were just testing the working directory resolution, it has no sense they were impacted. So I refactored that code in `StartLocatorCommand` & `StartServerCommand` extracting the working directory resolution code to a new method to be able to test it independently. You can review just what I changed for this in this commit:
`4cd39cc Refactor StartMemberUtils.resolveWorkingDir`
--
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] onichols-pivotal commented on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-827948647
> Tests were executed again today and one test failed unexpectedly. Last commit was done 21 days ago
It's always a risk to merge based on tests that passed long time ago (sometimes even short time ago -- a lot can change on develop in a short time). We recently had a commit merged that immediately broke develop, and found that PR checks had last been run 4 weeks earlier. To reduce this risk, I will be re-executing tests weekly to improve confidence that PRs have been tested against something more closely resembling the latest develop.
--
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] alb3rtobr closed pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
alb3rtobr closed pull request #6116:
URL: https://github.com/apache/geode/pull/6116
--
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] boglesby commented on a change in pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
boglesby commented on a change in pull request #6116:
URL: https://github.com/apache/geode/pull/6116#discussion_r605295413
##########
File path: geode-common/src/test/java/org/apache/geode/internal/inet/LocalHostUtilTest.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.inet;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.junit.Test;
+
+public class LocalHostUtilTest {
+
+ @Test
+ public void isWildCardAddressReturnsTrueIfAddressIsIPv4WildcardIP() {
+ assertThat(LocalHostUtil.isWildcardAddress("0.0.0.0")).isTrue();
+ }
+
+ @Test
+ public void isWildCardAddressReturnsTrueIfAddressIsIPv6WildcardIP() {
+ assertThat(LocalHostUtil.isWildcardAddress("::")).isTrue();
+ }
+
+ @Test
+ public void isWildCardAddressReturnsFalseIfAddressIsNull() {
+ assertThat(LocalHostUtil.isWildcardAddress(null)).isFalse();
+ }
+
+ @Test
+ public void isWildCardAddressReturnsFalseIfAddressIsEmpty() {
+ assertThat(LocalHostUtil.isWildcardAddress("")).isFalse();
+ }
+}
Review comment:
Do you need a test like:
```
@Test
public void isWildCardAddressReturnsFalseIfAddressIsNotWildcard() {
assertThat(LocalHostUtil.isWildcardAddress("1.2.3.4")).isFalse();
}
```
--
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] alb3rtobr commented on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-828250179
> > Tests were executed again today and one test failed unexpectedly. Last commit was done 21 days ago
>
> It's always a risk to merge based on tests that passed long time ago (sometimes even short time ago -- a lot can change on develop in a short time). We recently had a commit merged that immediately broke develop, and found that PR checks had last been run 4 weeks earlier. To reduce this risk, I will be re-executing tests weekly to improve confidence that PRs have been tested against something more closely resembling the latest develop.
Agree, but then PRs should be rebased against develop branch, right?
In my case, I have seen there are conflicts I have to solve, so Im going to rebase anyway...
--
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] alb3rtobr commented on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-813969370
@davebarnes97 Could you check if your "request for changes" can be removed? 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] albertogpz commented on a change in pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #6116:
URL: https://github.com/apache/geode/pull/6116#discussion_r594421863
##########
File path: geode-common/src/main/java/org/apache/geode/internal/inet/LocalHostUtil.java
##########
@@ -242,6 +242,10 @@ public static InetAddress getAnyLocalAddress() {
return new InetSocketAddress(0).getAddress();
}
+ public static boolean isWildcardAddress(String address) {
+ return (address != null && (address.equals("0.0.0.0") || address.equals("::")));
Review comment:
How about simplifying this as follows?:
return "0.0.0.0".equals(address) || "::".equals(address);
----------------------------------------------------------------
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] mhansonp commented on a change in pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6116:
URL: https://github.com/apache/geode/pull/6116#discussion_r602616553
##########
File path: geode-gfsh/src/integrationTest/java/org/apache/geode/management/internal/cli/GfshParserAutoCompletionIntegrationTest.java
##########
@@ -30,6 +30,12 @@
@Category(GfshTest.class)
public class GfshParserAutoCompletionIntegrationTest {
+ /**
+ * Number of @CliOption parameters of StartServerCommand.startServer()
+ * method +1 due to "--group" & "--groups" are defined in the same @CliOption
+ */
+ final int startServerCommandCliOptions = 55;
Review comment:
Can this be a calculated rather than magic number?
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/fd/GMSHealthMonitor.java
##########
@@ -953,8 +955,22 @@ public void init(Services<ID> s) throws MembershipConfigurationException {
public void started() throws MemberStartupException {
setLocalAddress(services.getMessenger().getMemberID());
try {
- serverSocket = createServerSocket(localAddress.getInetAddress(),
- services.getConfig().getMembershipPortRange());
+ InetAddress address = localAddress.getInetAddress();
Review comment:
I think it would be good to make this a separate function. This clutters up a function that doesn't really care.
--
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] mhansonp edited a comment on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
mhansonp edited a comment on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-814281957
> Thanks @mhansonp & @boglesby , I have added a commit after your comments.
I still approve of your changes. :) Thanks for making the suggested changes as well, they look good.
--
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] mhansonp commented on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-814281957
> Thanks @mhansonp & @boglesby , I have added a commit after your comments.
I still approve of your changes. Thanks for making the suggested changes as well, they look good.
--
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] alb3rtobr commented on a change in pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #6116:
URL: https://github.com/apache/geode/pull/6116#discussion_r600855043
##########
File path: geode-common/src/main/java/org/apache/geode/internal/inet/LocalHostUtil.java
##########
@@ -242,6 +242,10 @@ public static InetAddress getAnyLocalAddress() {
return new InetSocketAddress(0).getAddress();
}
+ public static boolean isWildcardAddress(String address) {
+ return (address != null && (address.equals("0.0.0.0") || address.equals("::")));
Review comment:
Good catch! I have just added a commit fixing this. 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] alb3rtobr commented on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-813967614
Thanks @mhansonp & @boglesby , I have added a commit after your comments.
--
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] davebarnes97 commented on a change in pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
davebarnes97 commented on a change in pull request #6116:
URL: https://github.com/apache/geode/pull/6116#discussion_r600693233
##########
File path: geode-docs/tools_modules/gfsh/command-pages/start.html.md.erb
##########
@@ -605,6 +621,14 @@ start server --name=value [--assign-buckets(=value)] [--bind-address=value]
<td> </td>
</tr>
<tr>
+<td><span class="keyword parmname">\-\-membership-bind-address</span></td>
+<td>Specifies the IP address to which the UDP membership-related traffic will be bound. If defined, it has precedence over the value of <code>bind-address</code>. Wildcard addresses are not accepted.
+<div class="note note">
+<b>Note:</b> By default, UDP membership-related traffic is bound to <code>bind-address</code> but there is one exception. In case the server is bound to all local addresses, UDP membership-related traffic will be bound to the local host machine's address by default.
Review comment:
Delete the note here, which is redundant, as it repeats the description of default behavior already explained in the --bind-address entry above.
--
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] onichols-pivotal commented on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-828279620
>then PRs should be rebased against develop branch, right?
PR checks do automatically rebase against latest develop, so just re-executing them is sufficient. However when there is a conflict then they will not run at all, so it's a good idea to manually rebase especially if GitHub is also showing conflicts.
--
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] alb3rtobr commented on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-828837263
I have removed two of the code improvements not related with the feature to separated tickets and PRs. I hope this will make the review of this PR easier.
--
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] davebarnes97 commented on a change in pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
davebarnes97 commented on a change in pull request #6116:
URL: https://github.com/apache/geode/pull/6116#discussion_r600690924
##########
File path: geode-docs/tools_modules/gfsh/command-pages/start.html.md.erb
##########
@@ -526,7 +538,11 @@ start server --name=value [--assign-buckets(=value)] [--bind-address=value]
</tr>
<tr>
<td><span class="keyword parmname">\-\-bind-address</span></td>
-<td>The IP address on which the server will be bound.</td>
+<td>The IP address on which the server will be bound. The '*' character is accepted for wildcard addresses.
+<div class="note note">
+<b>Note:</b> In case the server is bound to all local addresses, UDP membership-related traffic will be bound to the local host machine's address.
Review comment:
Slight rewrite to use present tense and clarify default:
When the server is bound to all local addresses, the default for UDP membership-related traffic is to bind to the local host machine's address.
--
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] alb3rtobr commented on pull request #6116: GEODE-8877: Configurable UDP membership address
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #6116:
URL: https://github.com/apache/geode/pull/6116#issuecomment-827887781
I have added an empty commit to retrigger CI because test were executed again today and one test failed unexpectedly. Last commit was done 21 days ago and tests were ok.
--
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