asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in hyracks[master]: Changed the IFrameWriter Contract
Date Wed, 16 Dec 2015 00:53:54 GMT
abdullah alamoudi has posted comments on this change.

Change subject: Changed the IFrameWriter Contract
......................................................................


Patch Set 2:

(42 comments)

I have addressed all the comments and added the test coverage to include Throwables that are
not Exceptions using UnkownError.

Pushing the patch in a few minutes.

https://asterix-gerrit.ics.uci.edu/#/c/551/2/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/std/PartitioningSplitOperatorDescriptor.java
File algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/std/PartitioningSplitOperatorDescriptor.java:

Line 48:  * This IFrameWriter doesn't follow the contract
> Why it doesn't/can't follow the contract? Can't it be fixed?
It can be fixed and I left this there to remind me to fix it. Now I have.
Done.


https://asterix-gerrit.ics.uci.edu/#/c/551/2/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/std/StreamSelectRuntimeFactory.java
File algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/std/StreamSelectRuntimeFactory.java:

Line 110:             }
> should you also overwrite fail()?  Because you have the state isOpen here t
You are right. 
Good catch and done.


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/comm/IFrameWriter.java
File hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/comm/IFrameWriter.java:

Line 36:  * processing. Once open() is called, the {@link IFrameWriter} is in the OPENED
> "...called, no matter successfully or not, the..."
Done


Line 42:  * <li>{@link IFrameWriter#nextFrame(ByteBuffer)} to provide data to the {@link
IFrameWriter}. The call returns normally on success and the {@link IFrameWriter} remains in
the OPENED state. On failure, the call throws a {@link HyracksDataException}, and the {@link
IFrameWriter} enters the FAILED state.</li>
> IMO: if the call throws a {@link HyracksDataException}, the the {@link IFra
Done


Line 48:  * Note: If the call to {@link IFrameWriter#open()} failed, the {@link IFrameWriter#close()}
is still called by the producer.
> "is still" --> "should still be"
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-hadoop/src/main/java/org/apache/hyracks/dataflow/hadoop/mapreduce/MapperOperatorDescriptor.java
File hyracks/hyracks-dataflow-hadoop/src/main/java/org/apache/hyracks/dataflow/hadoop/mapreduce/MapperOperatorDescriptor.java:

Line 261:                 } catch (Exception e) {
> Exception --> Throwable
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/LocalityAwarePartitionDataWriter.java
File hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/LocalityAwarePartitionDataWriter.java:

Line 105:                 } catch (Exception e) {
> Exception --> Throwable
Done


Line 107:                         failException = new HyracksDataException("Error during fail()
call to frame writer", e);
> if (failException == null) {	
Done


Line 131:                 } catch (Exception e) {
> Exception --> Throwable
Done


Line 139:                     } catch (Exception e) {
> Exception --> Throwable
Done


Line 142:                         }
> if (closeException == null) {	
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/MToNReplicatingConnectorDescriptor.java
File hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/MToNReplicatingConnectorDescriptor.java:

Line 71:                         } catch (Exception e) {
> Exception --> Throwable
Done


Line 73:                                 failException = new HyracksDataException("Exception
closing frame writer");
> if(failException == null)}{
Done


Line 91:                         } catch (Exception e) {
> Exception --> Throwable
Done


Line 93:                                 closeException = new HyracksDataException("Exception
closing frame writer");
> if(closeException == null)}{
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/PartitionDataWriter.java
File hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/PartitionDataWriter.java:

Line 72:                     } catch (Exception e) {
> Exception-->Throwable
Done


Line 75:                         }
> if(closeException == null)}{
Done


Line 81:                 } catch (Exception e) {
> Exception --> Throwable
Done


Line 82:                     if (closeException == null) {
> if(closeException == null)}{
Done


Line 130:                 } catch (Exception e) {
> Exception --> Throwable
Done


Line 132:                         failException = new HyracksDataException("Error during fail()
call");
> if(failException == null)}{
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/file/FileScanOperatorDescriptor.java
File hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/file/FileScanOperatorDescriptor.java:

Line 69:                 } catch (Exception e) {
> Exception -> Throwable
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/HybridHashJoinOperatorDescriptor.java
File hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/HybridHashJoinOperatorDescriptor.java:

Line 418:                     writer.open();
> move writer.open() to be the first line of this method?
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/InMemoryHashJoinOperatorDescriptor.java
File hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/InMemoryHashJoinOperatorDescriptor.java:

Line 233:                     writer.open();
> move up writer.open() a line?
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/NestedLoopJoinOperatorDescriptor.java
File hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/NestedLoopJoinOperatorDescriptor.java:

Line 197:                     writer.open();
> move up writer.open() a line?
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/OptimizedHybridHashJoinOperatorDescriptor.java
File hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/OptimizedHybridHashJoinOperatorDescriptor.java:

Line 381:                     writer.open();
> move up writer.open() a line?
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/misc/MaterializerTaskState.java
File hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/misc/MaterializerTaskState.java:

Line 78:         } catch (Exception e) {
> Exception-->Throwable
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/misc/SplitOperatorDescriptor.java
File hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/misc/SplitOperatorDescriptor.java:

Line 114:              * This IFrameWriter doesn't follow the contract
> Can't that be fixed?
It can be and it now is. I added the note to remind myself to fix it :D

Done.


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/rewriting/SuperActivityRewritingTest.java
File hyracks/hyracks-examples/hyracks-integration-tests/src/test/java/org/apache/hyracks/tests/rewriting/SuperActivityRewritingTest.java:

Line 87:                 } catch (Exception e) {
> Exception --> Throwable
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-hdfs/hyracks-hdfs-core/src/main/java/org/apache/hyracks/hdfs/dataflow/HDFSReadOperatorDescriptor.java
File hyracks/hyracks-hdfs/hyracks-hdfs-core/src/main/java/org/apache/hyracks/hdfs/dataflow/HDFSReadOperatorDescriptor.java:

Line 143:                 } catch (Exception e) {
> Exception -> Throwable
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-hdfs/hyracks-hdfs-core/src/main/java/org/apache/hyracks/hdfs2/dataflow/HDFSReadOperatorDescriptor.java
File hyracks/hyracks-hdfs/hyracks-hdfs-core/src/main/java/org/apache/hyracks/hdfs2/dataflow/HDFSReadOperatorDescriptor.java:

Line 158:                 } catch (Exception e) {
> Exception --> Throwable
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-storage-am-btree/src/test/java/org/apache/hyracks/storage/am/btree/test/FramewriterTest.java
File hyracks/hyracks-storage-am-btree/src/test/java/org/apache/hyracks/storage/am/btree/test/FramewriterTest.java:

Line 148:         // since we have two appenders, then we need to test each test twice 
> space
Done


Line 233:          *   
> space.
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java
File hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java:

Line 195:         } catch (Exception e) {
> Exception --> Throwable
Done


Line 196:             closeException = new HyracksDataException("Failed pushing frames to
next operator", e);
> closeException = new HyracksDataException(e);
Done


Line 213:                         closeException = new HyracksDataException("Failed to close
next operator", e);
> if(closeException == null)}{
Done


Line 226:                             throw new HyracksDataException("Failed to close next
operator", e);
> if(closeException == null)}{
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/TreeIndexDiskOrderScanOperatorNodePushable.java
File hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/TreeIndexDiskOrderScanOperatorNodePushable.java:

Line 88:             } catch (Exception e) {
> Exception --> Throwable
Done


Line 92:                 if (scanStarted) {
> scanStarted doesn't seem to be accurate enough to show the cursor is succes
Done. I removed this check completely. Still cursor.close() might be called before the cursor
is open. The assumption here is that the cursor should be smart enough to know that and throw
an exception.


Line 95:                     } catch(Exception cursorCloseException){
> Exception-->Throwable
Done


Line 101:         } catch (Exception e) {
> Throwable.
Done


https://asterix-gerrit.ics.uci.edu/#/c/551/2/hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/dataflow/BinaryTokenizerOperatorNodePushable.java
File hyracks/hyracks-storage-am-lsm-invertedindex/src/main/java/org/apache/hyracks/storage/am/lsm/invertedindex/dataflow/BinaryTokenizerOperatorNodePushable.java:

Line 38: /*
> Why it doesn't follow?  Can it be fixed?
Done. I fixed it but forgot to remove the comment.


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/551
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I03da090002f79f4db7b5b31454ce3ac2b9e40c7f
Gerrit-PatchSet: 2
Gerrit-Project: hyracks
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: Yingyi Bu <yingyib@google.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message