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/02/11 17:28:21 UTC

[GitHub] [accumulo] milleruntime opened a new pull request #1505: Optimize Tablet volume replacement

milleruntime opened a new pull request #1505: Optimize Tablet volume replacement
URL: https://github.com/apache/accumulo/pull/1505
 
 
   * Stop Tablet constructor from checking every file for volume
   replacements when feature not used

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [accumulo] EdColeman commented on a change in pull request #1505: Optimize Tablet volume replacement

Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1505: Optimize Tablet volume replacement
URL: https://github.com/apache/accumulo/pull/1505#discussion_r377831812
 
 

 ##########
 File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java
 ##########
 @@ -184,9 +184,8 @@ public static String switchRootTableVolume(String location) throws IOException {
    * for use it chooses a new tablet directory.
    */
   public static TabletFiles updateTabletVolumes(AccumuloServerContext context, ZooLock zooLock,
-      VolumeManager vm, KeyExtent extent, TabletFiles tabletFiles, boolean replicate)
-      throws IOException {
-    List<Pair<Path,Path>> replacements = ServerConstants.getVolumeReplacements();
+      VolumeManager vm, KeyExtent extent, TabletFiles tabletFiles, boolean replicate,
+      List<Pair<Path,Path>> replacements) throws IOException {
     log.trace("Using volume replacements: " + replacements);
 
 Review comment:
   Should the log statement be using parameter substitution? 
   `log.trace("Using volume replacements: {}" + replacements);`

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [accumulo] ctubbsii commented on issue #1505: Optimize Tablet volume replacement

Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1505: Optimize Tablet volume replacement
URL: https://github.com/apache/accumulo/pull/1505#issuecomment-594186071
 
 
   Because of #1525, this change ultimately got reverted from 1.10, and only applied to 2.1.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [accumulo] ivakegg commented on a change in pull request #1505: Optimize Tablet volume replacement

Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #1505: Optimize Tablet volume replacement
URL: https://github.com/apache/accumulo/pull/1505#discussion_r377831737
 
 

 ##########
 File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java
 ##########
 @@ -184,9 +184,8 @@ public static String switchRootTableVolume(String location) throws IOException {
    * for use it chooses a new tablet directory.
    */
   public static TabletFiles updateTabletVolumes(AccumuloServerContext context, ZooLock zooLock,
-      VolumeManager vm, KeyExtent extent, TabletFiles tabletFiles, boolean replicate)
-      throws IOException {
-    List<Pair<Path,Path>> replacements = ServerConstants.getVolumeReplacements();
 
 Review comment:
   +1

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [accumulo] milleruntime merged pull request #1505: Optimize Tablet volume replacement

Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1505: Optimize Tablet volume replacement
URL: https://github.com/apache/accumulo/pull/1505
 
 
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [accumulo] ctubbsii commented on a change in pull request #1505: Optimize Tablet volume replacement

Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1505: Optimize Tablet volume replacement
URL: https://github.com/apache/accumulo/pull/1505#discussion_r377831890
 
 

 ##########
 File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java
 ##########
 @@ -184,9 +184,8 @@ public static String switchRootTableVolume(String location) throws IOException {
    * for use it chooses a new tablet directory.
    */
   public static TabletFiles updateTabletVolumes(AccumuloServerContext context, ZooLock zooLock,
-      VolumeManager vm, KeyExtent extent, TabletFiles tabletFiles, boolean replicate)
-      throws IOException {
-    List<Pair<Path,Path>> replacements = ServerConstants.getVolumeReplacements();
 
 Review comment:
   Oh, right. I didn't see there was a return value.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [accumulo] milleruntime commented on a change in pull request #1505: Optimize Tablet volume replacement

Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1505: Optimize Tablet volume replacement
URL: https://github.com/apache/accumulo/pull/1505#discussion_r377830031
 
 

 ##########
 File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java
 ##########
 @@ -184,9 +184,8 @@ public static String switchRootTableVolume(String location) throws IOException {
    * for use it chooses a new tablet directory.
    */
   public static TabletFiles updateTabletVolumes(AccumuloServerContext context, ZooLock zooLock,
-      VolumeManager vm, KeyExtent extent, TabletFiles tabletFiles, boolean replicate)
-      throws IOException {
-    List<Pair<Path,Path>> replacements = ServerConstants.getVolumeReplacements();
 
 Review comment:
   Well we wouldn't be able to just return because the return value would wipe out the files and logs kept in tabletPaths.  But since that object gets passed I think we could just return it.

----------------------------------------------------------------
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


With regards,
Apache Git Services

[GitHub] [accumulo] ctubbsii commented on a change in pull request #1505: Optimize Tablet volume replacement

Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1505: Optimize Tablet volume replacement
URL: https://github.com/apache/accumulo/pull/1505#discussion_r377803422
 
 

 ##########
 File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java
 ##########
 @@ -184,9 +184,8 @@ public static String switchRootTableVolume(String location) throws IOException {
    * for use it chooses a new tablet directory.
    */
   public static TabletFiles updateTabletVolumes(AccumuloServerContext context, ZooLock zooLock,
-      VolumeManager vm, KeyExtent extent, TabletFiles tabletFiles, boolean replicate)
-      throws IOException {
-    List<Pair<Path,Path>> replacements = ServerConstants.getVolumeReplacements();
 
 Review comment:
   Would it make more sense to keep the replacements assignment in this method, but short-circuit the method with a `if (replacements.isEmpty()) { return; }` line?
   
   It seems like this is a smaller change and would keep the logic all together that handles the updates, rather than place the special case of no replacements outside the 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [accumulo] milleruntime commented on a change in pull request #1505: Optimize Tablet volume replacement

Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1505: Optimize Tablet volume replacement
URL: https://github.com/apache/accumulo/pull/1505#discussion_r377834567
 
 

 ##########
 File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java
 ##########
 @@ -184,9 +184,8 @@ public static String switchRootTableVolume(String location) throws IOException {
    * for use it chooses a new tablet directory.
    */
   public static TabletFiles updateTabletVolumes(AccumuloServerContext context, ZooLock zooLock,
-      VolumeManager vm, KeyExtent extent, TabletFiles tabletFiles, boolean replicate)
-      throws IOException {
-    List<Pair<Path,Path>> replacements = ServerConstants.getVolumeReplacements();
+      VolumeManager vm, KeyExtent extent, TabletFiles tabletFiles, boolean replicate,
+      List<Pair<Path,Path>> replacements) throws IOException {
     log.trace("Using volume replacements: " + replacements);
 
 Review comment:
   Yeah but I was avoiding making unrelated changes to keep the change to a minimum.

----------------------------------------------------------------
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


With regards,
Apache Git Services