abdullah alamoudi has posted comments on this change. Change subject: [ASTERIXDB-2204][STO] Fix implementations and usages of IIndexCursor ......................................................................
Patch Set 16: (15 comments) 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 Done 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 t Done 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. Done 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 an We promise when creating the destroyable that we will destroy it. All failures are going to be suppressed into the first failure. PS16, Line 34: [] > ... Done 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? Done.. This is the same class. I just moved it. Right now, there is only 1 ExceptionUtils in Hyracks but 3 in AsterixDB. PS16, Line 101: first.addSuppressed(second); > this loses interrupts I think that it can be dangerous to do Thread.currentThread().Interrupt() here. While we only use this method from the threads where the Exceptions where caught, What if someone uses it from some other thread. On the CC for example... Then the wrong thread will be interrupted. I am doing this but we need to be careful. 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() Done 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 Done PS16, Line 121: <version>2.0.2-beta</version> > move to dependency management in top-level hyracks pom Done 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 Done 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() Done 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-resour Done 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 Done https://asterix-gerrit.ics.uci.edu/#/c/2324/10/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/EnforcedIndexCursor.java File hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/EnforcedIndexCursor.java: PS10, Line 30: class > add an implementation with do nothing in the test class itself. This will m Done -- 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
