You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@parquet.apache.org by GitBox <gi...@apache.org> on 2020/12/15 14:39:41 UTC
[GitHub] [parquet-mr] StefanXiepj opened a new pull request #849: PARQUET-1954: TCP connection leak in parquet dump
StefanXiepj opened a new pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849
Make sure you have checked _all_ steps below.
### Jira
My PR addresses the following [PARQUET-1954](https://issues.apache.org/jira/browse/PARQUET-1954T/)
### Tests
does not need testing
----------------------------------------------------------------
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] [parquet-mr] Fokko commented on a change in pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#discussion_r549295644
##########
File path: parquet-tools/src/main/java/org/apache/parquet/tools/command/DumpCommand.java
##########
@@ -165,48 +165,47 @@ public static void dump(PrettyPrintWriter out, ParquetMetadata meta, MessageType
ParquetFileReader freader = null;
if (showmd) {
+ long group = 0;
+ for (BlockMetaData block : blocks) {
try {
- long group = 0;
- for (BlockMetaData block : blocks) {
- if (group != 0) out.println();
- out.format("row group %d%n", group++);
- out.rule('-');
-
-
- List<ColumnChunkMetaData> ccmds = block.getColumns();
- if (showColumns != null) {
- ccmds = new ArrayList<ColumnChunkMetaData>();
- for (ColumnChunkMetaData ccmd : block.getColumns()) {
- String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
- if (showColumns.contains(path)) {
- ccmds.add(ccmd);
- }
- }
- }
-
- MetadataUtils.showDetails(out, ccmds);
+ if (group != 0)
+ out.println();
+ out.format("row group %d%n", group++);
+ out.rule('-');
+
+ List<ColumnChunkMetaData> ccmds = block.getColumns();
+ if (showColumns != null) {
+ ccmds = new ArrayList<ColumnChunkMetaData>();
+ for (ColumnChunkMetaData ccmd : block.getColumns()) {
+ String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
+ if (showColumns.contains(path)) {
+ ccmds.add(ccmd);
+ }
+ }
+ }
- List<BlockMetaData> rblocks = Collections.singletonList(block);
- freader = new ParquetFileReader(
- conf, meta.getFileMetaData(), inpath, rblocks, columns);
- PageReadStore store = freader.readNextRowGroup();
- while (store != null) {
- out.incrementTabLevel();
- for (ColumnDescriptor column : columns) {
- out.println();
- dump(out, store, column);
- }
- out.decrementTabLevel();
+ MetadataUtils.showDetails(out, ccmds);
- store = freader.readNextRowGroup();
- }
- out.flushColumns();
+ List<BlockMetaData> rblocks = Collections.singletonList(block);
+ freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns);
+ PageReadStore store = freader.readNextRowGroup();
+ while (store != null) {
+ out.incrementTabLevel();
+ for (ColumnDescriptor column : columns) {
+ out.println();
+ dump(out, store, column);
}
+ out.decrementTabLevel();
+
+ store = freader.readNextRowGroup();
+ }
+ out.flushColumns();
} finally {
- if (freader != null) {
- freader.close();
- }
+ if (freader != null) {
+ freader.close();
Review comment:
The `ParquetFileReader` is Closeable, I would suggest using the try-with-resource pattern: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
As @gszadovszky already suggested.
----------------------------------------------------------------
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] [parquet-mr] StefanXiepj commented on a change in pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
StefanXiepj commented on a change in pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#discussion_r549592510
##########
File path: parquet-tools/src/main/java/org/apache/parquet/tools/command/DumpCommand.java
##########
@@ -165,48 +165,47 @@ public static void dump(PrettyPrintWriter out, ParquetMetadata meta, MessageType
ParquetFileReader freader = null;
if (showmd) {
+ long group = 0;
+ for (BlockMetaData block : blocks) {
try {
- long group = 0;
- for (BlockMetaData block : blocks) {
- if (group != 0) out.println();
- out.format("row group %d%n", group++);
- out.rule('-');
-
-
- List<ColumnChunkMetaData> ccmds = block.getColumns();
- if (showColumns != null) {
- ccmds = new ArrayList<ColumnChunkMetaData>();
- for (ColumnChunkMetaData ccmd : block.getColumns()) {
- String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
- if (showColumns.contains(path)) {
- ccmds.add(ccmd);
- }
- }
- }
-
- MetadataUtils.showDetails(out, ccmds);
+ if (group != 0)
+ out.println();
+ out.format("row group %d%n", group++);
+ out.rule('-');
+
+ List<ColumnChunkMetaData> ccmds = block.getColumns();
+ if (showColumns != null) {
+ ccmds = new ArrayList<ColumnChunkMetaData>();
+ for (ColumnChunkMetaData ccmd : block.getColumns()) {
+ String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
+ if (showColumns.contains(path)) {
+ ccmds.add(ccmd);
+ }
+ }
+ }
- List<BlockMetaData> rblocks = Collections.singletonList(block);
- freader = new ParquetFileReader(
- conf, meta.getFileMetaData(), inpath, rblocks, columns);
- PageReadStore store = freader.readNextRowGroup();
- while (store != null) {
- out.incrementTabLevel();
- for (ColumnDescriptor column : columns) {
- out.println();
- dump(out, store, column);
- }
- out.decrementTabLevel();
+ MetadataUtils.showDetails(out, ccmds);
- store = freader.readNextRowGroup();
- }
- out.flushColumns();
+ List<BlockMetaData> rblocks = Collections.singletonList(block);
+ freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns);
+ PageReadStore store = freader.readNextRowGroup();
+ while (store != null) {
+ out.incrementTabLevel();
+ for (ColumnDescriptor column : columns) {
+ out.println();
+ dump(out, store, column);
}
+ out.decrementTabLevel();
+
+ store = freader.readNextRowGroup();
+ }
+ out.flushColumns();
} finally {
- if (freader != null) {
- freader.close();
- }
+ if (freader != null) {
+ freader.close();
Review comment:
@Fokko thanks for your suggestion, can you review it again?
----------------------------------------------------------------
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] [parquet-mr] StefanXiepj commented on a change in pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
StefanXiepj commented on a change in pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#discussion_r549279770
##########
File path: parquet-tools/src/main/java/org/apache/parquet/tools/command/DumpCommand.java
##########
@@ -165,48 +165,47 @@ public static void dump(PrettyPrintWriter out, ParquetMetadata meta, MessageType
ParquetFileReader freader = null;
if (showmd) {
+ long group = 0;
+ for (BlockMetaData block : blocks) {
try {
- long group = 0;
- for (BlockMetaData block : blocks) {
- if (group != 0) out.println();
- out.format("row group %d%n", group++);
- out.rule('-');
-
-
- List<ColumnChunkMetaData> ccmds = block.getColumns();
- if (showColumns != null) {
- ccmds = new ArrayList<ColumnChunkMetaData>();
- for (ColumnChunkMetaData ccmd : block.getColumns()) {
- String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
- if (showColumns.contains(path)) {
- ccmds.add(ccmd);
- }
- }
- }
-
- MetadataUtils.showDetails(out, ccmds);
+ if (group != 0)
+ out.println();
+ out.format("row group %d%n", group++);
+ out.rule('-');
+
+ List<ColumnChunkMetaData> ccmds = block.getColumns();
+ if (showColumns != null) {
+ ccmds = new ArrayList<ColumnChunkMetaData>();
+ for (ColumnChunkMetaData ccmd : block.getColumns()) {
+ String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
+ if (showColumns.contains(path)) {
+ ccmds.add(ccmd);
+ }
+ }
+ }
- List<BlockMetaData> rblocks = Collections.singletonList(block);
- freader = new ParquetFileReader(
- conf, meta.getFileMetaData(), inpath, rblocks, columns);
- PageReadStore store = freader.readNextRowGroup();
- while (store != null) {
- out.incrementTabLevel();
- for (ColumnDescriptor column : columns) {
- out.println();
- dump(out, store, column);
- }
- out.decrementTabLevel();
+ MetadataUtils.showDetails(out, ccmds);
- store = freader.readNextRowGroup();
- }
- out.flushColumns();
+ List<BlockMetaData> rblocks = Collections.singletonList(block);
+ freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns);
+ PageReadStore store = freader.readNextRowGroup();
+ while (store != null) {
+ out.incrementTabLevel();
+ for (ColumnDescriptor column : columns) {
+ out.println();
+ dump(out, store, column);
}
+ out.decrementTabLevel();
+
+ store = freader.readNextRowGroup();
+ }
+ out.flushColumns();
} finally {
- if (freader != null) {
- freader.close();
- }
+ if (freader != null) {
+ freader.close();
Review comment:
hi, @gszadovszky Could you help me to review this PR ? thanks.
----------------------------------------------------------------
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] [parquet-mr] StefanXiepj commented on a change in pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
StefanXiepj commented on a change in pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#discussion_r543875687
##########
File path: parquet-tools/src/main/java/org/apache/parquet/tools/command/DumpCommand.java
##########
@@ -165,48 +165,47 @@ public static void dump(PrettyPrintWriter out, ParquetMetadata meta, MessageType
ParquetFileReader freader = null;
if (showmd) {
+ long group = 0;
+ for (BlockMetaData block : blocks) {
try {
- long group = 0;
- for (BlockMetaData block : blocks) {
- if (group != 0) out.println();
- out.format("row group %d%n", group++);
- out.rule('-');
-
-
- List<ColumnChunkMetaData> ccmds = block.getColumns();
- if (showColumns != null) {
- ccmds = new ArrayList<ColumnChunkMetaData>();
- for (ColumnChunkMetaData ccmd : block.getColumns()) {
- String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
- if (showColumns.contains(path)) {
- ccmds.add(ccmd);
- }
- }
- }
-
- MetadataUtils.showDetails(out, ccmds);
+ if (group != 0)
+ out.println();
+ out.format("row group %d%n", group++);
+ out.rule('-');
+
+ List<ColumnChunkMetaData> ccmds = block.getColumns();
+ if (showColumns != null) {
+ ccmds = new ArrayList<ColumnChunkMetaData>();
+ for (ColumnChunkMetaData ccmd : block.getColumns()) {
+ String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
+ if (showColumns.contains(path)) {
+ ccmds.add(ccmd);
+ }
+ }
+ }
- List<BlockMetaData> rblocks = Collections.singletonList(block);
- freader = new ParquetFileReader(
- conf, meta.getFileMetaData(), inpath, rblocks, columns);
- PageReadStore store = freader.readNextRowGroup();
- while (store != null) {
- out.incrementTabLevel();
- for (ColumnDescriptor column : columns) {
- out.println();
- dump(out, store, column);
- }
- out.decrementTabLevel();
+ MetadataUtils.showDetails(out, ccmds);
- store = freader.readNextRowGroup();
- }
- out.flushColumns();
+ List<BlockMetaData> rblocks = Collections.singletonList(block);
+ freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns);
+ PageReadStore store = freader.readNextRowGroup();
+ while (store != null) {
+ out.incrementTabLevel();
+ for (ColumnDescriptor column : columns) {
+ out.println();
+ dump(out, store, column);
}
+ out.decrementTabLevel();
+
+ store = freader.readNextRowGroup();
+ }
+ out.flushColumns();
} finally {
- if (freader != null) {
- freader.close();
- }
+ if (freader != null) {
+ freader.close();
Review comment:
This is the memory used before and after fix this bug:
----------------------------------------------------------------
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] [parquet-mr] StefanXiepj commented on pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
StefanXiepj commented on pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#issuecomment-745393587
@Pavitheran Could you review this PR , thanks.
----------------------------------------------------------------
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] [parquet-mr] gszadovszky commented on a change in pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
gszadovszky commented on a change in pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#discussion_r544152090
##########
File path: parquet-tools/src/main/java/org/apache/parquet/tools/command/DumpCommand.java
##########
@@ -165,48 +165,47 @@ public static void dump(PrettyPrintWriter out, ParquetMetadata meta, MessageType
ParquetFileReader freader = null;
if (showmd) {
+ long group = 0;
+ for (BlockMetaData block : blocks) {
try {
- long group = 0;
- for (BlockMetaData block : blocks) {
- if (group != 0) out.println();
- out.format("row group %d%n", group++);
- out.rule('-');
-
-
- List<ColumnChunkMetaData> ccmds = block.getColumns();
- if (showColumns != null) {
- ccmds = new ArrayList<ColumnChunkMetaData>();
- for (ColumnChunkMetaData ccmd : block.getColumns()) {
- String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
- if (showColumns.contains(path)) {
- ccmds.add(ccmd);
- }
- }
- }
-
- MetadataUtils.showDetails(out, ccmds);
+ if (group != 0)
+ out.println();
+ out.format("row group %d%n", group++);
+ out.rule('-');
+
+ List<ColumnChunkMetaData> ccmds = block.getColumns();
+ if (showColumns != null) {
+ ccmds = new ArrayList<ColumnChunkMetaData>();
+ for (ColumnChunkMetaData ccmd : block.getColumns()) {
+ String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
+ if (showColumns.contains(path)) {
+ ccmds.add(ccmd);
+ }
+ }
+ }
- List<BlockMetaData> rblocks = Collections.singletonList(block);
- freader = new ParquetFileReader(
- conf, meta.getFileMetaData(), inpath, rblocks, columns);
- PageReadStore store = freader.readNextRowGroup();
- while (store != null) {
- out.incrementTabLevel();
- for (ColumnDescriptor column : columns) {
- out.println();
- dump(out, store, column);
- }
- out.decrementTabLevel();
+ MetadataUtils.showDetails(out, ccmds);
- store = freader.readNextRowGroup();
- }
- out.flushColumns();
+ List<BlockMetaData> rblocks = Collections.singletonList(block);
+ freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns);
+ PageReadStore store = freader.readNextRowGroup();
+ while (store != null) {
+ out.incrementTabLevel();
+ for (ColumnDescriptor column : columns) {
+ out.println();
+ dump(out, store, column);
}
+ out.decrementTabLevel();
+
+ store = freader.readNextRowGroup();
+ }
+ out.flushColumns();
} finally {
- if (freader != null) {
- freader.close();
- }
+ if (freader != null) {
+ freader.close();
Review comment:
I did not say your fix is not working. What I suggest is to avoid closing a reader twice if an issue happens in the `ParquetFileReader` constructor.
----------------------------------------------------------------
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] [parquet-mr] gszadovszky commented on a change in pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
gszadovszky commented on a change in pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#discussion_r543485871
##########
File path: parquet-tools/src/main/java/org/apache/parquet/tools/command/DumpCommand.java
##########
@@ -165,48 +165,47 @@ public static void dump(PrettyPrintWriter out, ParquetMetadata meta, MessageType
ParquetFileReader freader = null;
if (showmd) {
+ long group = 0;
+ for (BlockMetaData block : blocks) {
try {
- long group = 0;
- for (BlockMetaData block : blocks) {
- if (group != 0) out.println();
- out.format("row group %d%n", group++);
- out.rule('-');
-
-
- List<ColumnChunkMetaData> ccmds = block.getColumns();
- if (showColumns != null) {
- ccmds = new ArrayList<ColumnChunkMetaData>();
- for (ColumnChunkMetaData ccmd : block.getColumns()) {
- String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
- if (showColumns.contains(path)) {
- ccmds.add(ccmd);
- }
- }
- }
-
- MetadataUtils.showDetails(out, ccmds);
+ if (group != 0)
+ out.println();
+ out.format("row group %d%n", group++);
+ out.rule('-');
+
+ List<ColumnChunkMetaData> ccmds = block.getColumns();
+ if (showColumns != null) {
+ ccmds = new ArrayList<ColumnChunkMetaData>();
+ for (ColumnChunkMetaData ccmd : block.getColumns()) {
+ String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
+ if (showColumns.contains(path)) {
+ ccmds.add(ccmd);
+ }
+ }
+ }
- List<BlockMetaData> rblocks = Collections.singletonList(block);
- freader = new ParquetFileReader(
- conf, meta.getFileMetaData(), inpath, rblocks, columns);
- PageReadStore store = freader.readNextRowGroup();
- while (store != null) {
- out.incrementTabLevel();
- for (ColumnDescriptor column : columns) {
- out.println();
- dump(out, store, column);
- }
- out.decrementTabLevel();
+ MetadataUtils.showDetails(out, ccmds);
- store = freader.readNextRowGroup();
- }
- out.flushColumns();
+ List<BlockMetaData> rblocks = Collections.singletonList(block);
+ freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns);
+ PageReadStore store = freader.readNextRowGroup();
+ while (store != null) {
+ out.incrementTabLevel();
+ for (ColumnDescriptor column : columns) {
+ out.println();
+ dump(out, store, column);
}
+ out.decrementTabLevel();
+
+ store = freader.readNextRowGroup();
+ }
+ out.flushColumns();
} finally {
- if (freader != null) {
- freader.close();
- }
+ if (freader != null) {
+ freader.close();
Review comment:
If an I/O exception occurs in the `ParquetFileReader` constructor then you might try to close the previous reader object. I would guess there is no problem with invoking `close` multiple times on the same object but who knows? It might be a good idea to reset the reference so you would not close the same object again. Another idea would be to use try-with-resources for the code part where the reader is really needed.
----------------------------------------------------------------
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] [parquet-mr] StefanXiepj commented on pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
StefanXiepj commented on pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#issuecomment-745968081
@wangyum Could you help me to review this PR ? thanks.
----------------------------------------------------------------
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] [parquet-mr] Fokko commented on a change in pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#discussion_r549295644
##########
File path: parquet-tools/src/main/java/org/apache/parquet/tools/command/DumpCommand.java
##########
@@ -165,48 +165,47 @@ public static void dump(PrettyPrintWriter out, ParquetMetadata meta, MessageType
ParquetFileReader freader = null;
if (showmd) {
+ long group = 0;
+ for (BlockMetaData block : blocks) {
try {
- long group = 0;
- for (BlockMetaData block : blocks) {
- if (group != 0) out.println();
- out.format("row group %d%n", group++);
- out.rule('-');
-
-
- List<ColumnChunkMetaData> ccmds = block.getColumns();
- if (showColumns != null) {
- ccmds = new ArrayList<ColumnChunkMetaData>();
- for (ColumnChunkMetaData ccmd : block.getColumns()) {
- String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
- if (showColumns.contains(path)) {
- ccmds.add(ccmd);
- }
- }
- }
-
- MetadataUtils.showDetails(out, ccmds);
+ if (group != 0)
+ out.println();
+ out.format("row group %d%n", group++);
+ out.rule('-');
+
+ List<ColumnChunkMetaData> ccmds = block.getColumns();
+ if (showColumns != null) {
+ ccmds = new ArrayList<ColumnChunkMetaData>();
+ for (ColumnChunkMetaData ccmd : block.getColumns()) {
+ String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
+ if (showColumns.contains(path)) {
+ ccmds.add(ccmd);
+ }
+ }
+ }
- List<BlockMetaData> rblocks = Collections.singletonList(block);
- freader = new ParquetFileReader(
- conf, meta.getFileMetaData(), inpath, rblocks, columns);
- PageReadStore store = freader.readNextRowGroup();
- while (store != null) {
- out.incrementTabLevel();
- for (ColumnDescriptor column : columns) {
- out.println();
- dump(out, store, column);
- }
- out.decrementTabLevel();
+ MetadataUtils.showDetails(out, ccmds);
- store = freader.readNextRowGroup();
- }
- out.flushColumns();
+ List<BlockMetaData> rblocks = Collections.singletonList(block);
+ freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns);
+ PageReadStore store = freader.readNextRowGroup();
+ while (store != null) {
+ out.incrementTabLevel();
+ for (ColumnDescriptor column : columns) {
+ out.println();
+ dump(out, store, column);
}
+ out.decrementTabLevel();
+
+ store = freader.readNextRowGroup();
+ }
+ out.flushColumns();
} finally {
- if (freader != null) {
- freader.close();
- }
+ if (freader != null) {
+ freader.close();
Review comment:
The `ParquetFileReader` is Closeable, I would suggest using the try-with-resource pattern: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
----------------------------------------------------------------
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] [parquet-mr] gszadovszky merged pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
gszadovszky merged pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849
----------------------------------------------------------------
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] [parquet-mr] StefanXiepj commented on a change in pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
StefanXiepj commented on a change in pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#discussion_r544782736
##########
File path: parquet-tools/src/main/java/org/apache/parquet/tools/command/DumpCommand.java
##########
@@ -165,48 +165,47 @@ public static void dump(PrettyPrintWriter out, ParquetMetadata meta, MessageType
ParquetFileReader freader = null;
if (showmd) {
+ long group = 0;
+ for (BlockMetaData block : blocks) {
try {
- long group = 0;
- for (BlockMetaData block : blocks) {
- if (group != 0) out.println();
- out.format("row group %d%n", group++);
- out.rule('-');
-
-
- List<ColumnChunkMetaData> ccmds = block.getColumns();
- if (showColumns != null) {
- ccmds = new ArrayList<ColumnChunkMetaData>();
- for (ColumnChunkMetaData ccmd : block.getColumns()) {
- String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
- if (showColumns.contains(path)) {
- ccmds.add(ccmd);
- }
- }
- }
-
- MetadataUtils.showDetails(out, ccmds);
+ if (group != 0)
+ out.println();
+ out.format("row group %d%n", group++);
+ out.rule('-');
+
+ List<ColumnChunkMetaData> ccmds = block.getColumns();
+ if (showColumns != null) {
+ ccmds = new ArrayList<ColumnChunkMetaData>();
+ for (ColumnChunkMetaData ccmd : block.getColumns()) {
+ String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
+ if (showColumns.contains(path)) {
+ ccmds.add(ccmd);
+ }
+ }
+ }
- List<BlockMetaData> rblocks = Collections.singletonList(block);
- freader = new ParquetFileReader(
- conf, meta.getFileMetaData(), inpath, rblocks, columns);
- PageReadStore store = freader.readNextRowGroup();
- while (store != null) {
- out.incrementTabLevel();
- for (ColumnDescriptor column : columns) {
- out.println();
- dump(out, store, column);
- }
- out.decrementTabLevel();
+ MetadataUtils.showDetails(out, ccmds);
- store = freader.readNextRowGroup();
- }
- out.flushColumns();
+ List<BlockMetaData> rblocks = Collections.singletonList(block);
+ freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns);
+ PageReadStore store = freader.readNextRowGroup();
+ while (store != null) {
+ out.incrementTabLevel();
+ for (ColumnDescriptor column : columns) {
+ out.println();
+ dump(out, store, column);
}
+ out.decrementTabLevel();
+
+ store = freader.readNextRowGroup();
+ }
+ out.flushColumns();
} finally {
- if (freader != null) {
- freader.close();
- }
+ if (freader != null) {
+ freader.close();
Review comment:
yeah yeah, thx for your review, i am sorry, please forgive me. Do you mean we can fix it like this:
```
...
} finally {
if (freader != null) {
freader.close();
// reset current reader is null, avoid closing a reader twice
freader = null;
}
}
```
or
```
try(ParquetFileReader freader =
new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns)){
// do something where the freader is really needed.
}
```
is right ?
----------------------------------------------------------------
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] [parquet-mr] StefanXiepj commented on a change in pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
StefanXiepj commented on a change in pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#discussion_r544782736
##########
File path: parquet-tools/src/main/java/org/apache/parquet/tools/command/DumpCommand.java
##########
@@ -165,48 +165,47 @@ public static void dump(PrettyPrintWriter out, ParquetMetadata meta, MessageType
ParquetFileReader freader = null;
if (showmd) {
+ long group = 0;
+ for (BlockMetaData block : blocks) {
try {
- long group = 0;
- for (BlockMetaData block : blocks) {
- if (group != 0) out.println();
- out.format("row group %d%n", group++);
- out.rule('-');
-
-
- List<ColumnChunkMetaData> ccmds = block.getColumns();
- if (showColumns != null) {
- ccmds = new ArrayList<ColumnChunkMetaData>();
- for (ColumnChunkMetaData ccmd : block.getColumns()) {
- String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
- if (showColumns.contains(path)) {
- ccmds.add(ccmd);
- }
- }
- }
-
- MetadataUtils.showDetails(out, ccmds);
+ if (group != 0)
+ out.println();
+ out.format("row group %d%n", group++);
+ out.rule('-');
+
+ List<ColumnChunkMetaData> ccmds = block.getColumns();
+ if (showColumns != null) {
+ ccmds = new ArrayList<ColumnChunkMetaData>();
+ for (ColumnChunkMetaData ccmd : block.getColumns()) {
+ String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
+ if (showColumns.contains(path)) {
+ ccmds.add(ccmd);
+ }
+ }
+ }
- List<BlockMetaData> rblocks = Collections.singletonList(block);
- freader = new ParquetFileReader(
- conf, meta.getFileMetaData(), inpath, rblocks, columns);
- PageReadStore store = freader.readNextRowGroup();
- while (store != null) {
- out.incrementTabLevel();
- for (ColumnDescriptor column : columns) {
- out.println();
- dump(out, store, column);
- }
- out.decrementTabLevel();
+ MetadataUtils.showDetails(out, ccmds);
- store = freader.readNextRowGroup();
- }
- out.flushColumns();
+ List<BlockMetaData> rblocks = Collections.singletonList(block);
+ freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns);
+ PageReadStore store = freader.readNextRowGroup();
+ while (store != null) {
+ out.incrementTabLevel();
+ for (ColumnDescriptor column : columns) {
+ out.println();
+ dump(out, store, column);
}
+ out.decrementTabLevel();
+
+ store = freader.readNextRowGroup();
+ }
+ out.flushColumns();
} finally {
- if (freader != null) {
- freader.close();
- }
+ if (freader != null) {
+ freader.close();
Review comment:
yeah yeah, thx for your review, i am sorry, please forgive me. Do you mean we can fix it like this:
`
...
} finally {
if (freader != null) {
freader.close();
// reset current reader is null, avoid closing a reader twice
freader = null;
}
}
`
or
`
try(ParquetFileReader freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns)){
// do something where the freader is really needed.
}
`
is right ?
----------------------------------------------------------------
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] [parquet-mr] StefanXiepj commented on a change in pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
StefanXiepj commented on a change in pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#discussion_r544782736
##########
File path: parquet-tools/src/main/java/org/apache/parquet/tools/command/DumpCommand.java
##########
@@ -165,48 +165,47 @@ public static void dump(PrettyPrintWriter out, ParquetMetadata meta, MessageType
ParquetFileReader freader = null;
if (showmd) {
+ long group = 0;
+ for (BlockMetaData block : blocks) {
try {
- long group = 0;
- for (BlockMetaData block : blocks) {
- if (group != 0) out.println();
- out.format("row group %d%n", group++);
- out.rule('-');
-
-
- List<ColumnChunkMetaData> ccmds = block.getColumns();
- if (showColumns != null) {
- ccmds = new ArrayList<ColumnChunkMetaData>();
- for (ColumnChunkMetaData ccmd : block.getColumns()) {
- String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
- if (showColumns.contains(path)) {
- ccmds.add(ccmd);
- }
- }
- }
-
- MetadataUtils.showDetails(out, ccmds);
+ if (group != 0)
+ out.println();
+ out.format("row group %d%n", group++);
+ out.rule('-');
+
+ List<ColumnChunkMetaData> ccmds = block.getColumns();
+ if (showColumns != null) {
+ ccmds = new ArrayList<ColumnChunkMetaData>();
+ for (ColumnChunkMetaData ccmd : block.getColumns()) {
+ String path = Joiner.on('.').skipNulls().join(ccmd.getPath().toArray());
+ if (showColumns.contains(path)) {
+ ccmds.add(ccmd);
+ }
+ }
+ }
- List<BlockMetaData> rblocks = Collections.singletonList(block);
- freader = new ParquetFileReader(
- conf, meta.getFileMetaData(), inpath, rblocks, columns);
- PageReadStore store = freader.readNextRowGroup();
- while (store != null) {
- out.incrementTabLevel();
- for (ColumnDescriptor column : columns) {
- out.println();
- dump(out, store, column);
- }
- out.decrementTabLevel();
+ MetadataUtils.showDetails(out, ccmds);
- store = freader.readNextRowGroup();
- }
- out.flushColumns();
+ List<BlockMetaData> rblocks = Collections.singletonList(block);
+ freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns);
+ PageReadStore store = freader.readNextRowGroup();
+ while (store != null) {
+ out.incrementTabLevel();
+ for (ColumnDescriptor column : columns) {
+ out.println();
+ dump(out, store, column);
}
+ out.decrementTabLevel();
+
+ store = freader.readNextRowGroup();
+ }
+ out.flushColumns();
} finally {
- if (freader != null) {
- freader.close();
- }
+ if (freader != null) {
+ freader.close();
Review comment:
yeah yeah, thx for your review, i am sorry, please forgive me. Do you mean we can fix it like this:
```
...
} finally {
if (freader != null) {
freader.close();
// reset current reader is null, avoid closing a reader twice
freader = null;
}
}
```
or
```
try(ParquetFileReader freader = new ParquetFileReader(conf, meta.getFileMetaData(), inpath, rblocks, columns)){
// do something where the freader is really needed.
}
```
is right ?
----------------------------------------------------------------
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] [parquet-mr] shangxinli commented on pull request #849: PARQUET-1954: TCP connection leak in parquet dump
Posted by GitBox <gi...@apache.org>.
shangxinli commented on pull request #849:
URL: https://github.com/apache/parquet-mr/pull/849#issuecomment-754286697
Good catch! Thanks for the effort!
----------------------------------------------------------------
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