You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/05/18 14:45:51 UTC
[GitHub] [accumulo] slackwinner opened a new pull request #2112: Improve LocalityGroupUtil.java
slackwinner opened a new pull request #2112:
URL: https://github.com/apache/accumulo/pull/2112
In response to issue #2041 , this commit contains minor changes to utilize Map.forEach() method instead of Map.entrySet().
--
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] [accumulo] ctubbsii commented on a change in pull request #2112: Improve LocalityGroupUtil.java
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2112:
URL: https://github.com/apache/accumulo/pull/2112#discussion_r634527549
##########
File path: core/src/main/java/org/apache/accumulo/core/util/LocalityGroupUtil.java
##########
@@ -377,11 +377,11 @@ public static void seek(FileSKVIterator reader, Range range, String lgName,
if (lgName == null) {
// this is the default locality group, create a set of all families not in the default group
Set<ByteSequence> nonDefaultFamilies = new HashSet<>();
- for (Entry<String,ArrayList<ByteSequence>> entry : localityGroupCF.entrySet()) {
- if (entry.getKey() != null) {
- nonDefaultFamilies.addAll(entry.getValue());
+ localityGroupCF.forEach((k, v) -> {
+ if (k != null) {
+ nonDefaultFamilies.addAll(v);
}
- }
+ });
Review comment:
Thanks for the analysis, @milleruntime . HashMap does support a single null key and multiple null values, but that doesn't necessarily mean it's a good idea. However, if there's any improvements needed to avoid a null key, it's unrelated to this PR.
--
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] [accumulo] slackwinner commented on pull request #2112: Improve LocalityGroupUtil.java
Posted by GitBox <gi...@apache.org>.
slackwinner commented on pull request #2112:
URL: https://github.com/apache/accumulo/pull/2112#issuecomment-843237397
Closing out this PR, to allow another developer to work on this issue.
--
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] [accumulo] slackwinner closed pull request #2112: Improve LocalityGroupUtil.java
Posted by GitBox <gi...@apache.org>.
slackwinner closed pull request #2112:
URL: https://github.com/apache/accumulo/pull/2112
--
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] [accumulo] ctubbsii merged pull request #2112: Improve LocalityGroupUtil.java
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2112:
URL: https://github.com/apache/accumulo/pull/2112
--
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] [accumulo] ctubbsii commented on pull request #2112: Improve LocalityGroupUtil.java
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2112:
URL: https://github.com/apache/accumulo/pull/2112#issuecomment-843405758
GitHub Actions seems to be having issues right now. I checked the build manually and it was fine.
--
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] [accumulo] ctubbsii commented on a change in pull request #2112: Improve LocalityGroupUtil.java
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2112:
URL: https://github.com/apache/accumulo/pull/2112#discussion_r634464641
##########
File path: core/src/main/java/org/apache/accumulo/core/util/LocalityGroupUtil.java
##########
@@ -377,11 +377,11 @@ public static void seek(FileSKVIterator reader, Range range, String lgName,
if (lgName == null) {
// this is the default locality group, create a set of all families not in the default group
Set<ByteSequence> nonDefaultFamilies = new HashSet<>();
- for (Entry<String,ArrayList<ByteSequence>> entry : localityGroupCF.entrySet()) {
- if (entry.getKey() != null) {
- nonDefaultFamilies.addAll(entry.getValue());
+ localityGroupCF.forEach((k, v) -> {
+ if (k != null) {
+ nonDefaultFamilies.addAll(v);
}
- }
+ });
Review comment:
Is it possible to have null keys in the first place? If not, that check is probably redundant, and we could just stream the values. Something like `localityGroupCF.values().stream().forEach(nonDefaultFamilies::addAll);`
If it is possible to have null keys, then your current code is probably fine.
--
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] [accumulo] slackwinner removed a comment on pull request #2112: Improve LocalityGroupUtil.java
Posted by GitBox <gi...@apache.org>.
slackwinner removed a comment on pull request #2112:
URL: https://github.com/apache/accumulo/pull/2112#issuecomment-843237397
Closing out this PR, to allow another developer to work on this issue.
--
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] [accumulo] milleruntime commented on a change in pull request #2112: Improve LocalityGroupUtil.java
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2112:
URL: https://github.com/apache/accumulo/pull/2112#discussion_r634508012
##########
File path: core/src/main/java/org/apache/accumulo/core/util/LocalityGroupUtil.java
##########
@@ -377,11 +377,11 @@ public static void seek(FileSKVIterator reader, Range range, String lgName,
if (lgName == null) {
// this is the default locality group, create a set of all families not in the default group
Set<ByteSequence> nonDefaultFamilies = new HashSet<>();
- for (Entry<String,ArrayList<ByteSequence>> entry : localityGroupCF.entrySet()) {
- if (entry.getKey() != null) {
- nonDefaultFamilies.addAll(entry.getValue());
+ localityGroupCF.forEach((k, v) -> {
+ if (k != null) {
+ nonDefaultFamilies.addAll(v);
}
- }
+ });
Review comment:
Looking at the code that calls this, it looks like the key can be null.
https://github.com/apache/accumulo/blob/0781550076f04d12716650fd64881d8b9d041afa/core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java#L260
It pulls `lgname` from the Map and then passes it in with the map. Based on the check before the loop, `if (lgName == null)` there will be at least one key that is definitely null.
--
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