Michael Blow has posted comments on this change. Change subject: [ASTERIXDB-2204][STO] Fix implementations and usages of IIndexCursor ......................................................................
Patch Set 16: -Code-Review (14 comments) partial review- submitting comments so far https://asterix-gerrit.ics.uci.edu/#/c/2324/16/asterixdb/asterix-external-data/pom.xml File asterixdb/asterix-external-data/pom.xml: PS16, Line 19: <project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; : xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> : <modelVersion>4.0.0</modelVersion> : <parent> : <artifactId>apache-asterixdb</artifactId> : <groupId>org.apache.asterix</groupId> : <version>0.9.3-SNAPSHOT</version> : </parent> : <licenses> : <license> : <name>Apache License, Version 2.0</name> : <url>http://www.apache.org/licenses/LICENSE-2.0.txt</url> : <distribution>repo</distribution> : <comments>A business-friendly OSS license</comments> : </license> : </licenses> : <artifactId>asterix-external-data</artifactId> : <properties> : <root.dir>${basedir}/..</root.dir> : <generatedSourcesDirectory>${project.build.directory}/generated-sources/lexer/</generatedSourcesDirectory> : </properties> : <build> : <plugins> : <plugin> : <groupId>org.apache.asterix</groupId> : <artifactId>lexer-generator-maven-plugin</artifactId> : <version>${project.version}</version> : <configuration> : <grammarFile>src/main/resources/adm.grammar</grammarFile> : <outputDir>${project.build.directory}/generated-sources/lexer/org/apache/asterix/runtime/operators/file/adm</outputDir> : </configuration> : <executions> : <execution> : <id>generate-lexer</id> : <phase>generate-sources</phase> : <goals> : <goal>generate-lexer</goal> : </goals> : </execution> : </executions> : </plugin> : <plugin> : <groupId>org.codehaus.mojo</groupId> : <artifactId>build-helper-maven-plugin</artifactId> : <executions> : <execution> : <id>add-source</id> : <phase>generate-sources</phase> : <goals> : <goal>add-source</goal> : </goals> : <configuration> : <sources> : <source>${project.build.directory}/generated-sources/lexer/</source> : </sources> : </configuration> : </execution> : </executions> : </plugin> : <plugin> : <groupId>org.jvnet.jaxb2.maven2</groupId> : <artifactId>maven-jaxb2-plugin</artifactId> : <executions> : <execution> : <id>configuration</id> : <goals> : <goal>generate</goal> : </goals> : <configuration> : <schemaDirectory>src/main/resources/schema</schemaDirectory> : <schemaIncludes> : <include>library.xsd</include> : </schemaIncludes> : <generatePackage>org.apache.asterix.external.library</generatePackage> : <generateDirectory>${project.build.directory}/generated-sources/configuration</generateDirectory> : </configuration> : </execution> : </executions> : </plugin> : <plugin> : <groupId>org.apache.maven.plugins</groupId> : <artifactId>maven-jar-plugin</artifactId> : <configuration> : <includes> : <include>**/*.class</include> : <include>**/*.txt</include> : <include>**/NOTICE</include> : <include>**/LICENSE</include> : <include>**/*.properties</include> : <include>**/services/**</include> : </includes> : </configuration> : <executions> : <execution> : <goals> : <goal>test-jar</goal> : </goals> : <phase>package</phase> : </execution> : </executions> : </plugin> : <plugin> : <artifactId>maven-assembly-plugin</artifactId> : <executions> : <execution> : <configuration> : <descriptors>src/main/assembly/binary-assembly-libzip.xml</descriptors> : </configuration> : <phase>package</phase> : <goals> : <goal>single</goal> : </goals> : </execution> : </executions> : </plugin> : <plugin> : <groupId>org.apache.maven.plugins</groupId> : <artifactId>maven-dependency-plugin</artifactId> : <configuration> : <ignoredUsedUndeclaredDependencies> : <ignoredUsedUndeclaredDependency>org.json:json:*</ignoredUsedUndeclaredDependency> : <ignoredUsedUndeclaredDependency>stax:stax-api:*</ignoredUsedUndeclaredDependency> : <ignoredUsedUndeclaredDependency>javax.xml.bind:jaxb-api:*</ignoredUsedUndeclaredDependency> : </ignoredUsedUndeclaredDependencies> : <ignoredUnusedDeclaredDependencies> : <ignoredUnusedDeclaredDependency>xml-apis:xml-apis:*</ignoredUnusedDeclaredDependency> : </ignoredUnusedDeclaredDependencies> : </configuration> : </plugin> : <plugin> : <groupId>org.apache.rat</groupId> : <artifactId>apache-rat-plugin</artifactId> : <configuration> : <licenses combine.children="append"> : <license implementation="org.apache.rat.analysis.license.SimplePatternBasedLicense"> : <licenseFamilyCategory>Kermit</licenseFamilyCategory> : <licenseFamilyName>Kermit Project</licenseFamilyName> : <notes>The UTF-8 sample "I Can Eat Glass" from The Kermit Project (license in LICENSE file)</notes> : <patterns>Copyright © 1981-2011, Trustees of Columbia University in the City of New York. All rights reserved.</patterns> : </license> : </licenses> : <licenseFamilies combine.children="append"> : <licenseFamily implementation="org.apache.rat.license.SimpleLicenseFamily"> : <familyName>Kermit Project</familyName> : </licenseFamily> : </licenseFamilies> : <excludes combine.children="append"> : <exclude>src/test/resources/classad/**</exclude> <!-- HTCondor (license in LICENSE file) --> : <exclude>src/test/resources/record.json</exclude> <!-- https://issues.apache.org/jira/browse/ASTERIXDB-1850 --> : <exclude>src/test/resources/change_feed.csv</exclude> : <exclude>src/test/resources/test_tweets.txt</exclude> : </excludes> : </configuration> : </plugin> : </plugins> : <pluginManagement> : <plugins> : <!--This plugin's configuration is used to store Eclipse m2e settings only. It has no influence on the Maven build itself.--> : <plugin> : <groupId>org.eclipse.m2e</groupId> : <artifactId>lifecycle-mapping</artifactId> : <version>1.0.0</version> : <configuration> : <lifecycleMappingMetadata> : <pluginExecutions> : <pluginExecution> : <pluginExecutionFilter> : <groupId> org.apache.asterix</groupId> : <artifactId> lexer-generator-maven-plugin</artifactId> : <versionRange>[0.0,)</versionRange> : <goals> : <goal>generate-lexer</goal> : </goals> : </pluginExecutionFilter> : <action> : <execute> : <runOnIncremental>false</runOnIncremental> : </execute> : </action> : </pluginExecution> : <pluginExecution> : <pluginExecutionFilter> : <groupId> org.codehaus.mojo</groupId> : <artifactId>build-helper-maven-plugin</artifactId> : <versionRange>[0.0,)</versionRange> : <goals> : <goal>add-source</goal> : </goals> : </pluginExecutionFilter> : <action> : <ignore /> : </action> : </pluginExecution> : </pluginExecutions> : </lifecycleMappingMetadata> : </configuration> : </plugin> : </plugins> : </pluginManagement> : </build> : <dependencies> : <dependency> : <groupId>org.apache.asterix</groupId> : <artifactId>asterix-om</artifactId> : <version>${project.version}</version> : <type>jar</type> : <scope>compile</scope> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-test-support</artifactId> : <scope>test</scope> : </dependency> : <dependency> : <groupId>org.apache.asterix</groupId> : <artifactId>asterix-runtime</artifactId> : <version>${project.version}</version> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-hdfs-core</artifactId> : </dependency> : <dependency> : <groupId>org.apache.asterix</groupId> : <artifactId>asterix-common</artifactId> : <version>${project.version}</version> : </dependency> : <dependency> : <groupId>org.apache.asterix</groupId> : <artifactId>asterix-active</artifactId> : <version>${project.version}</version> : </dependency> : <dependency> : <groupId>org.apache.asterix</groupId> : <artifactId>asterix-hivecompat</artifactId> : <version>${project.version}</version> : </dependency> : <dependency> : <groupId>org.twitter4j</groupId> : <artifactId>twitter4j-core</artifactId> : <version>4.0.3</version> : <scope>provided</scope> : </dependency> : <dependency> : <groupId>org.twitter4j</groupId> : <artifactId>twitter4j-stream</artifactId> : <version>4.0.3</version> : <scope>provided</scope> : </dependency> : <dependency> : <groupId>com.rometools</groupId> : <artifactId>rome-fetcher</artifactId> : </dependency> : <dependency> : <groupId>com.rometools</groupId> : <artifactId>rome</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hive</groupId> : <artifactId>hive-serde</artifactId> : </dependency> : <dependency> : <groupId>com.e-movimento.tinytools</groupId> : <artifactId>privilegedaccessor</artifactId> : <version>1.2.2</version> : <scope>test</scope> : </dependency> : <dependency> : <groupId>com.couchbase.client</groupId> : <artifactId>core-io</artifactId> : <version>1.3.2</version> : </dependency> : <dependency> : <groupId>org.mockito</groupId> : <artifactId>mockito-all</artifactId> : <version>2.0.2-beta</version> : <scope>test</scope> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-api</artifactId> : <type>test-jar</type> : <scope>test</scope> : </dependency> : <dependency> : <groupId>org.apache.wicket</groupId> : <artifactId>wicket-util</artifactId> : <version>1.5.2</version> : <scope>test</scope> : </dependency> : <dependency> : <groupId>commons-io</groupId> : <artifactId>commons-io</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-dataflow-common</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-storage-am-lsm-btree</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>algebricks-data</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>algebricks-core</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-storage-am-lsm-rtree</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>algebricks-runtime</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-storage-am-lsm-common</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-storage-common</artifactId> : </dependency> : <dependency> : <groupId>org.apache.httpcomponents</groupId> : <artifactId>httpclient</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-util</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>algebricks-common</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hadoop</groupId> : <artifactId>hadoop-common</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hadoop</groupId> : <artifactId>hadoop-mapreduce-client-core</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-storage-am-rtree</artifactId> : </dependency> : <dependency> : <groupId>org.apache.commons</groupId> : <artifactId>commons-lang3</artifactId> : </dependency> : <dependency> : <groupId>junit</groupId> : <artifactId>junit</artifactId> : <scope>test</scope> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-dataflow-std</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-data-std</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-storage-am-btree</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-storage-am-common</artifactId> : </dependency> : <dependency> : <groupId>org.apache.hyracks</groupId> : <artifactId>hyracks-api</artifactId> : </dependency> : <dependency> : <groupId>xml-apis</groupId> : <artifactId>xml-apis</artifactId> : <version>1.4.01</version> : </dependency> : <dependency> : <groupId>com.fasterxml.jackson.core</groupId> : <artifactId>jackson-databind</artifactId> : </dependency> : <dependency> : <groupId>com.fasterxml.jackson.core</groupId> : <artifactId>jackson-core</artifactId> : </dependency> : <dependency> : <groupId>org.apache.commons</groupId> : <artifactId>commons-collections4</artifactId> : <version>4.1</version> : </dependency> : <dependency> : <groupId>org.apache.logging.log4j</groupId> : <artifactId>log4j-api</artifactId> : </dependency> : </dependencies> : </project> revert https://asterix-gerrit.ics.uci.edu/#/c/2324/16/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/indexing/ExternalFileIndexAccessor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/indexing/ExternalFileIndexAccessor.java: PS16, Line 139: Throwable failure = null; : if (fileIndexSearchCursor != null) { : try { : fileIndexSearchCursor.close(); : } catch (Throwable th) { // NOSONAR Will be re-thrown : failure = th; : } : try { : fileIndexSearchCursor.destroy(); : } catch (Throwable th) {// NOSONAR Will be re-thrown : failure = ExceptionUtils.suppress(failure, th); : } : } : if (fileIndexAccessor != null) { : try { : fileIndexAccessor.destroy(); : } catch (Throwable th) {// NOSONAR Will be re-thrown : failure = ExceptionUtils.suppress(failure, th); : } : } : if (index != null) { : try { : indexDataflowHelper.close(); : } catch (Throwable th) {// NOSONAR Will be re-thrown : failure = ExceptionUtils.suppress(failure, th); : } : } : if (failure != null) { : throw HyracksDataException.create(failure); : } this loses interrupts, and is dangerous wrt Errors. Can we add something that works like try-with-resources? https://asterix-gerrit.ics.uci.edu/#/c/2324/16/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/IDestroyable.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/IDestroyable.java: PS16, Line 29: must throw exceptions is undefined? seems like a strict contract to mandate throwing exceptions... https://asterix-gerrit.ics.uci.edu/#/c/2324/16/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/util/DestroyUtils.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/util/DestroyUtils.java: PS16, Line 32: return the first exception thrown by the destroy calls, or null if no exception was thrown this seems risky, that exceptions can be lost- can we make this throw on any exception? PS16, Line 34: [] ... https://asterix-gerrit.ics.uci.edu/#/c/2324/16/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/util/ExceptionUtils.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/util/ExceptionUtils.java: PS16, Line 31: ExceptionUtils merge org.apache.hyracks.control.common.utils.ExceptionUtils into this? PS16, Line 101: first.addSuppressed(second); this loses interrupts https://asterix-gerrit.ics.uci.edu/#/c/2324/16/hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTreeOpContext.java File hyracks-fullstack/hyracks/hyracks-storage-am-btree/src/main/java/org/apache/hyracks/storage/am/btree/impls/BTreeOpContext.java: PS16, Line 395: Throwable failure = null; : try { : accessor.destroy(); : } catch (Throwable e) { // NOSONAR Will be re-thrown : failure = e; : } finally { : try { : if (cursor != null) { : cursor.destroy(); : } : } catch (Throwable e) { // NOSONAR Will be re-thrown : failure = ExceptionUtils.suppress(failure, e); : } : } : if (failure != null) { : throw HyracksDataException.create(failure); : } HyracksDataException.suppress() https://asterix-gerrit.ics.uci.edu/#/c/2324/16/hyracks-fullstack/hyracks/hyracks-storage-am-common/pom.xml File hyracks-fullstack/hyracks/hyracks-storage-am-common/pom.xml: PS16, Line 45: <configuration> : <includes> : <include>**/*.class</include> : <include>**/*.properties</include> : <include>**/README*</include> : <include>**/NOTICE*</include> : <include>**/LICENSE*</include> : <include>**/DEPENDENCIES*</include> : </includes> : </configuration> remove PS16, Line 121: <version>2.0.2-beta</version> move to dependency management in top-level hyracks pom https://asterix-gerrit.ics.uci.edu/#/c/2324/16/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java: PS16, Line 229: } catch (Throwable th) { // NOSONAR Must ensure root failure is not lost agree with sonar here, remove this NOSONAR and fix PS16, Line 222: Throwable failure = null; : try { : failure = releaseResources(); : } finally { : try { : // will definitely be called regardless of exceptions : writer.close(); : } catch (Throwable th) { // NOSONAR Must ensure root failure is not lost : failure = ExceptionUtils.suppress(failure, th); : } : } : if (failure != null) { : throw HyracksDataException.create(failure); : } HyracksDataException.suppress() PS16, Line 239: Throwable failure = null; : if (index != null) { : // if index == null, then the index open was not successful : if (!failed) { : try { : if (appender.getTupleCount() > 0) { : appender.write(writer, true); : } : } catch (Throwable th) { // NOSONAR Must ensure writer.fail is called. : // subsequently, the failure will be thrown : failure = th; : } : if (failure != null) { : try { : writer.fail(); : } catch (Throwable th) {// NOSONAR Must cursor.close is called. : // subsequently, the failure will be thrown : failure = ExceptionUtils.suppress(failure, th); : } : } : } : if (cursor != null) { : try { : cursor.close(); : } catch (Throwable th) {// NOSONAR Must ensure cursor.destroy is called. : // subsequently, the failure will be thrown : failure = ExceptionUtils.suppress(failure, th); : } : try { : cursor.destroy(); : } catch (Throwable th) { // NOSONAR Must ensure indexAccessor.destroy is called. : // subsequently, the failure will be thrown : failure = ExceptionUtils.suppress(failure, th); : } : } : if (indexAccessor != null) { : try { : indexAccessor.destroy(); : } catch (Throwable th) { // NOSONAR Must ensure root failure is not lost : failure = ExceptionUtils.suppress(failure, th); : } : } : try { : indexHelper.close(); : } catch (Throwable th) { // NOSONAR Must ensure root failure is not lost : failure = ExceptionUtils.suppress(failure, th); : } : } : return failure; this seems scary- can we create something that behaves like try-with-resources to do this cleanup, that doesn't lose interrupts? https://asterix-gerrit.ics.uci.edu/#/c/2324/16/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/TreeIndexDiskOrderScanOperatorNodePushable.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/TreeIndexDiskOrderScanOperatorNodePushable.java: PS16, Line 61: Throwable failure = null; : treeIndexHelper.open(); : try { : writer.open(); : FrameTupleAppender appender = new FrameTupleAppender(new VSizeFrame(ctx)); : scan(appender); : appender.write(writer, true); : } catch (Throwable th) { // NOSONAR: Must call writer.fail : failure = th; : try { : writer.fail(); : } catch (Throwable failFailure) {// NOSONAR: Must maintain all stacks : failure = ExceptionUtils.suppress(failure, failFailure); : } : } finally { : try { : writer.close(); : } catch (Throwable closeFailure) { // // NOSONAR: Must call maintain root failure : failure = ExceptionUtils.suppress(failure, closeFailure); : } : } : if (failure != null) { : throw HyracksDataException.create(failure); : } HyracksDataException.suppress -- To view, visit https://asterix-gerrit.ics.uci.edu/2324 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I98a7a8b931eb24dbe11bf2bdc61b754ca28ebdf9 Gerrit-PatchSet: 16 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Luo Chen <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
