You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/02/04 01:01:15 UTC
[GitHub] [cassandra] frankgh opened a new pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
frankgh opened a new pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438
A NullPointerException is possible when the StartupClusterConnectivityChecker fails,
and one of the missing peers has been added/discovered after the initialization of
the map between peers and datacenters. This is causing in some rare instances the
NullPointerException because an "element cannot be mapped to a null key".
This commit prevents the NullPointerException, by checking the map first for the DC
and if it's null, it calls the `getDatacenterSource` function, which is a call to
the `DatabaseDescriptor.getEndpointSnitch()::getDatacenter` method.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic closed pull request #1438: CASSANDRA-17347 Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] frankgh commented on a change in pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
frankgh commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799532333
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -171,7 +172,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
{
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peerToDatacenter::get,
+ .collect(groupingBy(peer -> {
+ String dc = peerToDatacenter.get(peer);
+ return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
Review comment:
we could use only JDK APIs, like this alternatively:
```java
Optional.ofNullable(peerToDatacenter.get(peer)).orElseGet(() -> Optional.ofNullable(getDatacenterSource.apply(peer)).orElse("unknown"))
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a change in pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799371360
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -171,7 +172,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
{
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peerToDatacenter::get,
+ .collect(groupingBy(peer -> {
+ String dc = peerToDatacenter.get(peer);
+ return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
Review comment:
can be reworked like this:
```
.collect(groupingBy(peer -> peerToDatacenter.getOrDefault(peer, StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown")),
mapping(InetAddressAndPort::getHostAddressAndPort, toList())));
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] frankgh commented on a change in pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
frankgh commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799549978
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -170,12 +171,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
}
else
{
+
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peer -> {
- String dc = peerToDatacenter.get(peer);
- return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
- },
+ .collect(groupingBy(peer -> Optional.ofNullable(peerToDatacenter.get(peer)).orElseGet(() -> StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown")),
Review comment:
I have reverted it back to the simple null checking
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] frankgh commented on a change in pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
frankgh commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799526047
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -171,7 +172,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
{
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peerToDatacenter::get,
+ .collect(groupingBy(peer -> {
+ String dc = peerToDatacenter.get(peer);
+ return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
Review comment:
Yeah, I was trying to avoid calling the snitch. I pushed an update. Java 9 has an API to do what I'm trying to do here: https://docs.oracle.com/javase/9/docs/api/java/util/Objects.html#requireNonNullElseGet-T-java.util.function.Supplier-
However, I'm not sure if apache commons or guava offer something similar. I will spend some time looking for 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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a change in pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799371360
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -171,7 +172,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
{
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peerToDatacenter::get,
+ .collect(groupingBy(peer -> {
+ String dc = peerToDatacenter.get(peer);
+ return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
Review comment:
can be reworked like this:
```
.collect(groupingBy(peer -> peerToDatacenter.getOrDefault(peer, StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown")),
mapping(InetAddressAndPort::getHostAddressAndPort, toList())));
```
EDIT: However, after think about it a little bit, I have to admit that I am computing it unnecessarilly if it does not return null which differs with your approach.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a change in pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799545789
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -170,12 +171,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
}
else
{
+
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peer -> {
- String dc = peerToDatacenter.get(peer);
- return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
- },
+ .collect(groupingBy(peer -> Optional.ofNullable(peerToDatacenter.get(peer)).orElseGet(() -> StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown")),
Review comment:
@frankgh that is too verbose to my taste. Nothing wrong with what you had there before.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] frankgh commented on a change in pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
frankgh commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799529465
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -171,7 +172,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
{
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peerToDatacenter::get,
+ .collect(groupingBy(peer -> {
+ String dc = peerToDatacenter.get(peer);
+ return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
Review comment:
Ok. I am using `Optional.ofNullable` now. Let me know what you think @smiklosovic
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a change in pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799371360
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -171,7 +172,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
{
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peerToDatacenter::get,
+ .collect(groupingBy(peer -> {
+ String dc = peerToDatacenter.get(peer);
+ return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
Review comment:
can be reworked like this:
```
.collect(groupingBy(peer -> peerToDatacenter.getOrDefault(peer, StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown")),
mapping(InetAddressAndPort::getHostAddressAndPort, toList())));
```
EDIT: However, I have to admit that I am computing it unnecessarilly if it does not return null which differs with your approach.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a change in pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799371360
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -171,7 +172,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
{
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peerToDatacenter::get,
+ .collect(groupingBy(peer -> {
+ String dc = peerToDatacenter.get(peer);
+ return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
Review comment:
can be reworked like this:
```
.collect(groupingBy(peer -> peerToDatacenter.getOrDefault(peer,
StringUtils.defaultString(getDatacenterSource.apply(peer),
"unknown")),
mapping(InetAddressAndPort::getHostAddressAndPort,
toList())));
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] yifan-c commented on a change in pull request #1438: CASSANDRA-17347 Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
yifan-c commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799729740
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -171,7 +172,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
{
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peerToDatacenter::get,
+ .collect(groupingBy(peer -> {
+ String dc = peerToDatacenter.get(peer);
+ return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
Review comment:
The goal is here very simple, i.e. try to resolve the value from the map cache, if not try to resolve from snitch, if all attempts fail, use the default "unknown".
There are so many ways to express it in the code and all correctly. Functional, ternary operator, or just the plain `if`
In this particular scenario, I would rate the plain `if` to have the best readability, then the ternary operator. Functional seems a bit overkill.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a change in pull request #1438: Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799543307
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -171,7 +172,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
{
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peerToDatacenter::get,
+ .collect(groupingBy(peer -> {
+ String dc = peerToDatacenter.get(peer);
+ return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
Review comment:
I think it has to compile on Java 8 for sure. I recently hit a bug when a developer used `Set.of` which is not in 8 but in 11 (hence 9 maybe?). So please code against Java 8 in general as that is the lowest common denominator.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] frankgh commented on a change in pull request #1438: CASSANDRA-17347 Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
frankgh commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r800927741
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -170,12 +171,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
}
else
{
+
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peer -> {
- String dc = peerToDatacenter.get(peer);
- return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
- },
+ .collect(groupingBy(peer -> Optional.ofNullable(peerToDatacenter.get(peer)).orElseGet(() -> StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown")),
Review comment:
@smiklosovic I have pushed one update that favors readability. I think we can go with this version of the 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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] frankgh commented on a change in pull request #1438: CASSANDRA-17347 Fix NullPointerException during StartupClusterConnectivityChecker
Posted by GitBox <gi...@apache.org>.
frankgh commented on a change in pull request #1438:
URL: https://github.com/apache/cassandra/pull/1438#discussion_r799738846
##########
File path: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java
##########
@@ -171,7 +172,10 @@ public boolean execute(Set<InetAddressAndPort> peers, Function<InetAddressAndPor
{
// dc -> missing peer host addresses
Map<String, List<String>> peersDown = acks.getMissingPeers().stream()
- .collect(groupingBy(peerToDatacenter::get,
+ .collect(groupingBy(peer -> {
+ String dc = peerToDatacenter.get(peer);
+ return dc != null ? dc : StringUtils.defaultString(getDatacenterSource.apply(peer), "unknown");
Review comment:
I agree that plain `if` is more readable. I'm going with that approach
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org