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/25 19:14:49 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request #2118: PopulateMetadataTable Improvements
DomGarguilo opened a new pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118
Closes #1091
Improvements include:
1. Setting `currentRow` after each mutation
2. Adding a try-with-resources block
3. Consolidating two if statements
--
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] DomGarguilo commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r644921215
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -121,63 +114,56 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Text currentRow = null;
int dirCount = 0;
- while (true) {
- key.readFields(in);
- val.readFields(in);
+ try (DataInputStream in = new DataInputStream(new BufferedInputStream(zis))) {
Review comment:
I think you may be right. It is changes back in e883b1f
--
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] keith-turner commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r644841848
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -121,63 +114,56 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Text currentRow = null;
int dirCount = 0;
- while (true) {
- key.readFields(in);
- val.readFields(in);
+ try (DataInputStream in = new DataInputStream(new BufferedInputStream(zis))) {
Review comment:
Seems like this could close zis, which would not be desired inside the loop. I am uncertain about the actual behavior though.
--
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] Manno15 commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r640862589
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -91,15 +91,11 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Path path = new Path(tableInfo.exportFile);
- BatchWriter mbw = null;
- ZipInputStream zis = null;
-
- try {
- VolumeManager fs = manager.getVolumeManager();
-
- mbw = manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
-
- zis = new ZipInputStream(fs.open(path));
+ try (
+ BatchWriter mbw =
+ manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
+ VolumeManager fs = manager.getVolumeManager();
Review comment:
Possible. How much have you looked into it? I noticed in the test `SortedLogRecoveryTest`, it has the VolumeManager creation in a try with resources block but that is only using the local fs for testing purposes. I do not see anywhere else in the code where that is done, however.
--
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] DomGarguilo commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r640873796
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -91,15 +91,11 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Path path = new Path(tableInfo.exportFile);
- BatchWriter mbw = null;
- ZipInputStream zis = null;
-
- try {
- VolumeManager fs = manager.getVolumeManager();
-
- mbw = manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
-
- zis = new ZipInputStream(fs.open(path));
+ try (
+ BatchWriter mbw =
+ manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
+ VolumeManager fs = manager.getVolumeManager();
Review comment:
Im looking into it now. In this case it looks like it gets the VolumeManager from Manager which is probably needed elsewhere. Also, VolumeManager.close() has a comment saying:
> close the underlying FileSystems
Which leads me to believe it will close more than intended.
--
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] keith-turner commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r640877001
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -91,15 +91,11 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Path path = new Path(tableInfo.exportFile);
- BatchWriter mbw = null;
- ZipInputStream zis = null;
-
- try {
- VolumeManager fs = manager.getVolumeManager();
-
- mbw = manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
-
- zis = new ZipInputStream(fs.open(path));
+ try (
+ BatchWriter mbw =
+ manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
+ VolumeManager fs = manager.getVolumeManager();
Review comment:
I don't think this should be closed, I think its a shared object in the master that would be used by other threads and task.
--
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] Manno15 commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r640878917
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -91,15 +91,11 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Path path = new Path(tableInfo.exportFile);
- BatchWriter mbw = null;
- ZipInputStream zis = null;
-
- try {
- VolumeManager fs = manager.getVolumeManager();
-
- mbw = manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
-
- zis = new ZipInputStream(fs.open(path));
+ try (
+ BatchWriter mbw =
+ manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
+ VolumeManager fs = manager.getVolumeManager();
Review comment:
Yeah, after looking into it more, I agree.
--
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] DomGarguilo commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r640894019
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -91,15 +91,11 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Path path = new Path(tableInfo.exportFile);
- BatchWriter mbw = null;
- ZipInputStream zis = null;
-
- try {
- VolumeManager fs = manager.getVolumeManager();
-
- mbw = manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
-
- zis = new ZipInputStream(fs.open(path));
+ try (
+ BatchWriter mbw =
+ manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
+ VolumeManager fs = manager.getVolumeManager();
Review comment:
Agreed. Made this change in the most recent commit, 3aa1bdf.
--
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] DomGarguilo commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r640800351
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -91,15 +91,11 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Path path = new Path(tableInfo.exportFile);
- BatchWriter mbw = null;
- ZipInputStream zis = null;
-
- try {
- VolumeManager fs = manager.getVolumeManager();
-
- mbw = manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
-
- zis = new ZipInputStream(fs.open(path));
+ try (
+ BatchWriter mbw =
+ manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
+ VolumeManager fs = manager.getVolumeManager();
Review comment:
[Recent discussion in another ticket](https://github.com/apache/accumulo/pull/2117#discussion_r639859548) makes me think this probably should be moved outside of the try-with-resources block so it is not closed.
--
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] Manno15 merged pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
Manno15 merged pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118
--
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] Manno15 merged pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
Manno15 merged pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118
--
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] Manno15 commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r640878917
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -91,15 +91,11 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Path path = new Path(tableInfo.exportFile);
- BatchWriter mbw = null;
- ZipInputStream zis = null;
-
- try {
- VolumeManager fs = manager.getVolumeManager();
-
- mbw = manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
-
- zis = new ZipInputStream(fs.open(path));
+ try (
+ BatchWriter mbw =
+ manager.getContext().createBatchWriter(MetadataTable.NAME, new BatchWriterConfig());
+ VolumeManager fs = manager.getVolumeManager();
Review comment:
Yeah, after looking into it more, I agree. I see that the Tserver will call the close function for the filesystem when it gets shut down.
--
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] DomGarguilo commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r644921215
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -121,63 +114,56 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Text currentRow = null;
int dirCount = 0;
- while (true) {
- key.readFields(in);
- val.readFields(in);
+ try (DataInputStream in = new DataInputStream(new BufferedInputStream(zis))) {
Review comment:
I think you may be right. I'll change it back in the next commit.
--
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] DomGarguilo commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r644921215
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -121,63 +114,56 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Text currentRow = null;
int dirCount = 0;
- while (true) {
- key.readFields(in);
- val.readFields(in);
+ try (DataInputStream in = new DataInputStream(new BufferedInputStream(zis))) {
Review comment:
I think you may be right. I'll change it back in the next commit.
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -121,63 +114,56 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Text currentRow = null;
int dirCount = 0;
- while (true) {
- key.readFields(in);
- val.readFields(in);
+ try (DataInputStream in = new DataInputStream(new BufferedInputStream(zis))) {
Review comment:
I think you may be right. It is changes back in e883b1f
--
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] keith-turner commented on a change in pull request #2118: PopulateMetadataTable Improvements
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2118:
URL: https://github.com/apache/accumulo/pull/2118#discussion_r644841848
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/tableImport/PopulateMetadataTable.java
##########
@@ -121,63 +114,56 @@ static void readMappingFile(VolumeManager fs, ImportedTableInfo tableInfo, Strin
Text currentRow = null;
int dirCount = 0;
- while (true) {
- key.readFields(in);
- val.readFields(in);
+ try (DataInputStream in = new DataInputStream(new BufferedInputStream(zis))) {
Review comment:
Seems like this could close zis, which would not be desired inside the loop. I am uncertain about the actual behavior though.
--
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