You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@asterixdb.apache.org by "Till Westmann (Code Review)" <do...@asterixdb.incubator.apache.org> on 2016/12/05 05:36:04 UTC
Change in asterixdb-bad[master]: Updated to match code changes to asterix
Till Westmann has posted comments on this change.
Change subject: Updated to match code changes to asterix
......................................................................
Patch Set 15:
(19 comments)
A first round of comments.
https://asterix-gerrit.ics.uci.edu/#/c/1227/15/.gitignore
File .gitignore:
Line 3: asteri-opt-bom/target
Shouldn't those be covered by "target" below?
Line 12: *.hprof
Are these actually created by the build?
https://asterix-gerrit.ics.uci.edu/#/c/1227/15//COMMIT_MSG
Commit Message:
Line 7: Updated to match code changes to asterix
Would be nice to have a better description here.
https://asterix-gerrit.ics.uci.edu/#/c/1227/15/asterix-bad/src/main/java/org/apache/asterix/bad/ChannelJobService.java
File asterix-bad/src/main/java/org/apache/asterix/bad/ChannelJobService.java:
Line 41: import org.json.JSONException;
We shouldn't use org.json anymore - the license is category X now.
Line 60: e.printStackTrace();
Could we at least log this with a log4j logger?
Line 113: //TODO: Allow Repetitive Channels to use YMD durations
WS
Line 125:
empty line
Line 137:
empty line
Line 163: } catch (Exception e) {
Remove the try-catch?
Line 167: throw new Exception();
Give an error message?
Line 186: System.out.println(response.toString());
Log to a logger?
Line 189: throw new Exception();
Give an error message?
Line 193: LOGGER.info("Broker connection failed to write");
Pass the exception it?
https://asterix-gerrit.ics.uci.edu/#/c/1227/15/asterix-bad/src/main/java/org/apache/asterix/bad/metadata/Procedure.java
File asterix-bad/src/main/java/org/apache/asterix/bad/metadata/Procedure.java:
Line 96:
empty lines?
https://asterix-gerrit.ics.uci.edu/#/c/1227/15/asterix-bad/src/main/java/org/apache/asterix/bad/runtime/RepetitiveChannelOperatorDescriptor.java
File asterix-bad/src/main/java/org/apache/asterix/bad/runtime/RepetitiveChannelOperatorDescriptor.java:
Line 65: } catch (Exception e) {
Can't we just throw HyracksDataException from the beginning?
https://asterix-gerrit.ics.uci.edu/#/c/1227/15/asterix-bad/src/main/java/org/apache/asterix/bad/runtime/RepetitiveChannelOperatorNodePushable.java
File asterix-bad/src/main/java/org/apache/asterix/bad/runtime/RepetitiveChannelOperatorNodePushable.java:
Line 62:
Huh?
https://asterix-gerrit.ics.uci.edu/#/c/1227/15/asterix-bad/src/main/resources/lang-extension/lang.txt
File asterix-bad/src/main/resources/lang-extension/lang.txt:
Line 59: | "broker" pairId = QualifiedName() ifExists = IfExists()
WS
https://asterix-gerrit.ics.uci.edu/#/c/1227/15/asterix-bad/src/test/resources/runtimets/queries/channel/drop_channel_check_metadata/drop_channel_check_metadata.3.query.aql
File asterix-bad/src/test/resources/runtimets/queries/channel/drop_channel_check_metadata/drop_channel_check_metadata.3.query.aql:
Line 3: for $result in dataset Metadata.Channel
WS
https://asterix-gerrit.ics.uci.edu/#/c/1227/15/asterix-opt-bom/pom.xml
File asterix-opt-bom/pom.xml:
Line 20: xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
Formatting - WS and indentation.
--
To view, visit https://asterix-gerrit.ics.uci.edu/1227
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I010b81776543e127f09f046a8601bb7184f7de9a
Gerrit-PatchSet: 15
Gerrit-Project: asterixdb-bad
Gerrit-Branch: master
Gerrit-Owner: Steven Jacobs <sj...@ucr.edu>
Gerrit-Reviewer: Ian Maxon <im...@apache.org>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Steven Jacobs <sj...@ucr.edu>
Gerrit-Reviewer: Till Westmann <ti...@apache.org>
Gerrit-Reviewer: abdullah alamoudi <ba...@gmail.com>
Gerrit-HasComments: Yes