You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/03/31 23:45:46 UTC
[GitHub] [accumulo] keith-turner opened a new pull request, #3271: Performs some minor clean of TabletLocator
keith-turner opened a new pull request, #3271:
URL: https://github.com/apache/accumulo/pull/3271
This commit has no changes in functionality, it only contains some minor code cleanup of the TabletLocator code.
The following changes were made.
* Encapsulated fields in TabletLocation class
* Replaced a Callable interface with a Supplier interface to avoid the uneeded exception thrown by Callable
* Removed an uneeded compareTo method from TabletLocation class
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3271: Performs some minor cleanup of TabletLocator
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3271:
URL: https://github.com/apache/accumulo/pull/3271#issuecomment-1492729431
Created this PR to pull some of refactoring done in #3143 into its own 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3271: Performs some minor cleanup of TabletLocator
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3271:
URL: https://github.com/apache/accumulo/pull/3271#discussion_r1155962011
##########
server/base/src/test/java/org/apache/accumulo/server/client/BulkImporterTest.java:
##########
@@ -147,25 +148,25 @@ public void testFindOverlappingTablets() throws Exception {
List<TabletLocation> overlaps =
BulkImporter.findOverlappingTablets(context, vm, locator, new Path(file), null, null, cs);
assertEquals(5, overlaps.size());
- Collections.sort(overlaps);
- assertEquals(new KeyExtent(tableId, new Text("a"), null), overlaps.get(0).tablet_extent);
+ Collections.sort(overlaps, Comparator.comparing(tl -> tl.getExtent()));
Review Comment:
Yeah this test was the only thing that used the compare to functionality. So I thought it would be simpler from a maintenance stand point to move it to the test. Also the test only needs to order by extent, it does not need any secondary comparison.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3271: Performs some minor cleanup of TabletLocator
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3271:
URL: https://github.com/apache/accumulo/pull/3271#issuecomment-1494355668
@ctubbsii and @dlmarion I went ahead and merged this because I have other TabletLocator clean ups that I want to extract from #3143 as their own commits. Its easiest to do that if this if already merged into main. Also this PR is mainly automated refactoring from an IDE. If there are any other changes you would like to see related to this PR let me know and I can make them in a follow on 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner merged pull request #3271: Performs some minor cleanup of TabletLocator
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #3271:
URL: https://github.com/apache/accumulo/pull/3271
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3271: Performs some minor cleanup of TabletLocator
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3271:
URL: https://github.com/apache/accumulo/pull/3271#discussion_r1155842057
##########
server/base/src/test/java/org/apache/accumulo/server/client/BulkImporterTest.java:
##########
@@ -147,25 +148,25 @@ public void testFindOverlappingTablets() throws Exception {
List<TabletLocation> overlaps =
BulkImporter.findOverlappingTablets(context, vm, locator, new Path(file), null, null, cs);
assertEquals(5, overlaps.size());
- Collections.sort(overlaps);
- assertEquals(new KeyExtent(tableId, new Text("a"), null), overlaps.get(0).tablet_extent);
+ Collections.sort(overlaps, Comparator.comparing(tl -> tl.getExtent()));
Review Comment:
Interested in this as well. Other than this, LGTM.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3271: Performs some minor cleanup of TabletLocator
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3271:
URL: https://github.com/apache/accumulo/pull/3271#discussion_r1155024601
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/RootTabletLocator.java:
##########
@@ -56,11 +56,12 @@ public <T extends Mutation> void binMutations(ClientContext context, List<T> mut
Map<String,TabletServerMutations<T>> binnedMutations, List<T> failures) {
TabletLocation rootTabletLocation = getRootTabletLocation(context);
if (rootTabletLocation != null) {
- TabletServerMutations<T> tsm = new TabletServerMutations<>(rootTabletLocation.tablet_session);
+ TabletServerMutations<T> tsm =
+ new TabletServerMutations<>(rootTabletLocation.getTserverSession());
Review Comment:
I'm not sure if this fits on one line after this suggestion or not, but this would be a good use of the `var` keyword, to avoid the redundant long class name in the variable declaration.
```suggestion
var tsm = new TabletServerMutations<T>(rootTabletLocation.getTserverSession());
```
##########
server/base/src/test/java/org/apache/accumulo/server/client/BulkImporterTest.java:
##########
@@ -147,25 +148,25 @@ public void testFindOverlappingTablets() throws Exception {
List<TabletLocation> overlaps =
BulkImporter.findOverlappingTablets(context, vm, locator, new Path(file), null, null, cs);
assertEquals(5, overlaps.size());
- Collections.sort(overlaps);
- assertEquals(new KeyExtent(tableId, new Text("a"), null), overlaps.get(0).tablet_extent);
+ Collections.sort(overlaps, Comparator.comparing(tl -> tl.getExtent()));
Review Comment:
The previous Comparable compared by extent, then by location, then by session. A few questions:
1. Did those secondary sorts not really matter?
2. Did I miss something, or is this test the only time we care about these being ordered? Was that Comparable stuff only there for this test?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org