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 2020/09/15 21:33:48 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request #1705: Clean up a few forEach loops
ctubbsii opened a new pull request #1705:
URL: https://github.com/apache/accumulo/pull/1705
Miscellaneous minor clean up found while working on unrelated code,
including:
* Using forEach loops on collections to streamline loops
* Inline one-time-use simple private methods
* Remove braces in some simple one-statement lambdas
----------------------------------------------------------------
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] EdColeman commented on a change in pull request #1705: Clean up a few forEach loops
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1705:
URL: https://github.com/apache/accumulo/pull/1705#discussion_r489037453
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/FileManager.java
##########
@@ -372,7 +368,8 @@ private void releaseReaders(KeyExtent tablet, List<FileSKVIterator> readers,
for (FileSKVIterator reader : readers) {
String fileName = reservedReaders.remove(reader);
if (!sawIOException)
- getFileList(fileName, openFiles).add(new OpenReader(fileName, reader));
Review comment:
Possibly add a comment to document the method reason?
----------------------------------------------------------------
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] EdColeman commented on a change in pull request #1705: Clean up a few forEach loops
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1705:
URL: https://github.com/apache/accumulo/pull/1705#discussion_r489033568
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchWriter.java
##########
@@ -509,19 +509,13 @@ private void updateAuthorizationFailures(Map<KeyExtent,SecurityErrorCode> author
synchronized (this) {
somethingFailed = true;
- mergeAuthorizationFailures(this.authorizationFailures, authorizationFailures);
Review comment:
Would a comment be appropriate? The method name explains what is happening, when merged it looks harder to reason why this is being done.
----------------------------------------------------------------
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 #1705: Clean up a few forEach loops
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1705:
URL: https://github.com/apache/accumulo/pull/1705#discussion_r489084780
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/FileManager.java
##########
@@ -372,7 +368,8 @@ private void releaseReaders(KeyExtent tablet, List<FileSKVIterator> readers,
for (FileSKVIterator reader : readers) {
String fileName = reservedReaders.remove(reader);
if (!sawIOException)
- getFileList(fileName, openFiles).add(new OpenReader(fileName, reader));
Review comment:
For this one, the previous method was an undocumented private one-liner with an unhelpful name that was used only once. I inline'd it automatically with the IDE and verified that the automated inline was performed correctly.
I have no idea what any of this code in this entire class is trying to do, without possibly hours of investigation, as none of it has helpful comments at all. At this point, I don't think I could come up with anything other than the existing mediocre `put files in openFiles` comment at the top of the method. I'm disinclined to spend the time it would require to investigate enough to be capable of adding a more useful comment, so I would prefer to defer to the next meaningful refactor of this code. :smiley_cat:
----------------------------------------------------------------
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 #1705: Clean up a few forEach loops
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1705:
URL: https://github.com/apache/accumulo/pull/1705#issuecomment-693094109
> I did not see any issues with a quick first pass - however, with some of the lambda conversions the variable names are changed and I'd need to look at more than the diffs provided to convince myself that the behavior is the same. If these were IDE generated, then they are more likely to not have an error. If this is still open tomorrow I'll try to make time for additional review.
@EdColeman Most of the changes here were automated IDE refactors, but a few were manual. I've examined them all very carefully, and most are trivial anyway. This change didn't break any tests, so I'm just going to merge and move on. Feel free to add additional review if you wish, but I'm content with the tests not being broken and my manual review. I did add the one comment you suggested, though. The other was... well... I replied to your suggestion above :smiley_cat:
----------------------------------------------------------------
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 #1705: Clean up a few forEach loops
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1705:
URL: https://github.com/apache/accumulo/pull/1705#discussion_r489076836
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchWriter.java
##########
@@ -509,19 +509,13 @@ private void updateAuthorizationFailures(Map<KeyExtent,SecurityErrorCode> author
synchronized (this) {
somethingFailed = true;
- mergeAuthorizationFailures(this.authorizationFailures, authorizationFailures);
Review comment:
Sure, I can add 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #1705: Clean up a few forEach loops
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1705:
URL: https://github.com/apache/accumulo/pull/1705
----------------------------------------------------------------
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 #1705: Clean up a few forEach loops
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1705:
URL: https://github.com/apache/accumulo/pull/1705#issuecomment-692992619
These are trivial changes, but I had them leftover from a previous working branch I had for unrelated changes, and figured I could get rid of that branch by just merging these in.
----------------------------------------------------------------
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