abdullah alamoudi has posted comments on this change.

Change subject: Enabled Feed Tests and Added External Library tests
......................................................................


Patch Set 12:

(115 comments)

https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/data/external-parser/dropbox/jobads1.txt
File asterix-app/data/external-parser/dropbox/jobads1.txt:

Line 3:         BlockWrites = 0; 
> trailing whitespace?
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java
File 
asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java:

Line 92:      * @return a map from dataverse -> list of uninstalled libraries. 
> WS
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/java/org/apache/asterix/test/common/TestLibrarian.java
File 
asterix-app/src/test/java/org/apache/asterix/test/common/TestLibrarian.java:

Line 37: import net.lingala.zip4j.core.ZipFile;
> Make sure you add this to NOTICE. It's ASL2 but not from the ASF.
Done


Line 54:         if (destinationDir.exists()) {
> Replace this if block by
Done


Line 76:     public static void rmDirRecursively(String path) throws 
IOException {
> Remove this method.
Done


Line 93:     public static void removeLibraryDir() throws IOException {
> Can there be a sanity check here about what's returned from getLibraryInsta
Mmmm. Shouldn't protecting against that be the responsibility of the OS?
Not sure how to whitelist this but I am giving it a try.


Line 95:         if (installLibDir.exists()) {
> Replace this if block by FileUtils.deleteQuietly(installLibDir);
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.1.ddl.aql
File 
asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.1.ddl.aql:

Line 22:                   Associate with the feed an external user-defined 
function. The UDF 
> WS
Done


Line 23:                   finds topics in each tweet. A topic is identified by 
a #. 
> WS
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.3.ddl.aql
File 
asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.3.ddl.aql:

Line 22:                   Associate with the feed an external user-defined 
function. The UDF 
> WS
Done


Line 23:                   finds topics in each tweet. A topic is identified by 
a #. 
> WS
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.4.update.aql
File 
asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.4.update.aql:

Line 22:                   Associate with the feed an external user-defined 
function. The UDF 
> WS
Done


Line 23:                   finds topics in each tweet. A topic is identified by 
a #. 
> WS
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.5.query.aql
File 
asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.5.query.aql:

Line 22:                   Associate with the feed an external user-defined 
function. The UDF 
> WS
Done


Line 23:                   finds topics in each tweet. A topic is identified by 
a #. 
> WS
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.1.ddl.aql
File 
asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.1.ddl.aql:

Line 20:  * Description  : Create an adapter that uses external parser to parse 
data from files
> Update description
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.3.ddl.aql
File 
asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.3.ddl.aql:

Line 19: 
> Just to be consistent, add the test description header.
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.4.update.aql
File 
asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.4.update.aql:

Line 20:  * Description  : Create a feed dataset and verify contents in Metadata
> Update description and date
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.5.query.aql
File 
asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.5.query.aql:

Line 18:  */
> Add description
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.7.ddl.aql
File 
asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.7.ddl.aql:

Line 20:  * Description  : Create a feed dataset and verify contents in Metadata
> update description
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_09/feeds_09.4.ddl.aql
File 
asterix-app/src/test/resources/runtimets/queries/feeds/feeds_09/feeds_09.4.ddl.aql:

Line 19: /*
> WSes
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_10/feeds_10.4.ddl.aql
File 
asterix-app/src/test/resources/runtimets/queries/feeds/feeds_10/feeds_10.4.ddl.aql:

Line 20:  * Description  : Create a feed using the synthetic feed simulator 
adapter. 
> WSes
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_12/feeds_12.4.ddl.aql
File 
asterix-app/src/test/resources/runtimets/queries/feeds/feeds_12/feeds_12.4.ddl.aql:

Line 20:  * Description  : Create a feed using the synthetic feed simulator 
adapter. 
> WSes
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/queries/feeds/issue_230_feeds/issue_230_feeds.4.ddl.aql
File 
asterix-app/src/test/resources/runtimets/queries/feeds/issue_230_feeds/issue_230_feeds.4.ddl.aql:

Line 19: /*
> WSes
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/src/test/resources/runtimets/testsuite.xml
File asterix-app/src/test/resources/runtimets/testsuite.xml:

Line 62:         </test-case>
> It seems like a lot of these are still broken, maybe we can file an issue a
I did not close the already filed issue for feed tests and added a comment to 
double check those.
most likely, they will go away since there are things that are fundamentally 
broken with them.


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java
File asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java:

Line 540: 
> Delete this to avoid PASSED being printed twice in the case of txnqar
Done


Line 601:                         case "lib": // expected format 
<dataverse-name> <library-name> <library-directory>
> Space is not a legal character for any of dataverse, library name, library 
It is legal. but then taking care of this seems like a lot of effort not well 
spent since this is strictly test case thing. However, I am adding a todo.


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/adapter/factory/GenericAdapterFactory.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/adapter/factory/GenericAdapterFactory.java:

Line 79:     public synchronized IDataSourceAdapter 
createAdapter(IHyracksTaskContext ctx, int partition) throws Exception {
> why does this have to be synchronized? who shared the Factory object?
All task partitions share the factory. This is fine but we shouldn't need this 
to be synchronized. However, in case of dynamically loaded members, we need to 
re-create and re-configure them.

because we don't have a way to serialize/deserialize these classes' objects 
within a hyracks job.


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/api/IRecordFlowController.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/api/IRecordFlowController.java:

Line 21: public interface IRecordFlowController<T> extends IDataFlowController {
> Unless you expect future uses for this interface, just delete it and replac
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedRecordDataFlowController.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedRecordDataFlowController.java:

Line 40:     protected AtomicBoolean closed = new AtomicBoolean(false);
> final
Done


Line 74:             th.printStackTrace();
> Is this temporary or accidental?  Should we have a TODO to remove?
Done


Line 85:             th.printStackTrace();
> Is this temporary or accidental?  Should we have a TODO to remove?
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedRuntimeInputHandler.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedRuntimeInputHandler.java:

Line 284:                         //mBuffer.sendReport(frame);
> Why is this commented out?  Is it no longer applicable (i.e. should we just
Still applicable but there is a bug there. I commented it out when I was trying 
to understand some behavior. re-enabled but added a TODO to fix.


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/AbstractStreamRecordReader.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/AbstractStreamRecordReader.java:

Line 98:         reader.setFeedLogManager(feedLogManager);
> Just a note that if this method is called before setInputStream, then this 
That happened to me a few times when I was writing this. It should never happen 
though.


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/EmptyLineSeparatedRecordReader.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/EmptyLineSeparatedRecordReader.java:

Line 24: 
> So maybe this is a stupid question, but why doesn't this situation count as
Not stupid at all. the name is confusing. with delimited text, each two records 
are separated by a new line.

With this one, each two records are separated by an empty line. I am using it 
for the old condor format where each line has an attribute and an empty line 
means end of record.


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/BasicInputStream.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/BasicInputStream.java:

Line 96:     public void setFeedLogManager(FeedLogManager logManager) {
> Make the super implementation non abstract with unsupportedOp and remove th
Mmmm, I have made some changes and each feed/feed_inputstream will get assigned 
a log manager. whether they decide to make use of it or not is up to the 
implementation.


Line 100:     public void setController(AbstractFeedDataFlowController 
controller) {
> Make the super implementation non abstract with unsupportedOp and remove th
Mmmm, I have made some changes and each feed/feed_inputstream will get assigned 
a reference to the data flow controller. whether they decide to make use of it 
or not is up to the implementation.


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/SocketInputStream.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/SocketInputStream.java:

Line 111:     public void setFeedLogManager(FeedLogManager logManager) {
> Make the super implementation non abstract with unsupportedOp and remove th
Done


Line 115:     public void setController(AbstractFeedDataFlowController 
controller) {
> Make the super implementation non abstract with unsupportedOp and remove th
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/HDFSInputStreamProvider.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/HDFSInputStreamProvider.java:

Line 126:         public void setFeedLogManager(FeedLogManager logManager) {
> Provide default implementation with unsupportedOp and remove this
Done


Line 130:         public void setController(AbstractFeedDataFlowController 
controller) {
> Make the super implementation non abstract with unsupportedOp and remove th
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/TwitterFirehoseInputStreamProvider.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/TwitterFirehoseInputStreamProvider.java:

Line 122:         public void setFeedLogManager(FeedLogManager logManager) {
> Make the super implementation non abstract with unsupportedOp and remove th
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/parser/ADMDataParser.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/parser/ADMDataParser.java:

Line 87:     private FeedLogManager feedLogManager;
> Remove this from here.
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/provider/DataflowControllerProvider.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/provider/DataflowControllerProvider.java:

Line 63:      * @param feedLogFileSplits 
> trailing whitespace (and next line)
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataCompatibilityUtils.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataCompatibilityUtils.java:

Line 77:                             "Unspecified (\"reader\" or \"format\") 
parameter for local file system adapter");
> Prefer "filesystem" myself, but I guess both are acceptable.
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/util/FeedLogManager.java
File 
asterix-external-data/src/main/java/org/apache/asterix/external/util/FeedLogManager.java:

Line 142:         recordLogger.newLine();
> Each call to recordLogger is pretty heavy, includes synchronization, using 
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/AMutableCharArrayString.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/AMutableCharArrayString.java:

Line 130:         System.arraycopy(otherString.toCharArray(), 0, value, 0, 
length);
> Should use String.getChars() to avoid the temporary char [] allocation (toC
Done


Line 336:         System.arraycopy(aString.toCharArray(), 0, value, i, 
aString.length());
> Should use String.getChars() to avoid the temporary char [] allocation (toC
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/AttributeReference.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/AttributeReference.java:

Line 78:      * 
> trailing ws
Done


Line 90:      * 
> trailing ws
Done


Line 113:      * 
> trailing ws
Done


Line 132:                 // Will this check result in infinite recursion? How 
do I stop it? 
> trailing ws
Done


Line 234:         // lookup with scope; this may side-affect state        
> trailing ws
Done


Line 402:      * 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/BuiltinClassAdFunctions.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/BuiltinClassAdFunctions.java:

Line 58:             if (name.equalsIgnoreCase("isundefined")) {
> equalsIgnoreCase is not very efficient when used over and over I think- rep
Done


Line 109:             // undefined and we're supposed to test for strict 
comparison, the 
> trailing ws
Done


Line 234:                 // Either take the number if it's the first, 
> trailing ws
Done


Line 305:                 // For this element of the list, make sure it is 
> trailing ws
Done


Line 363:             } else if (!stringValue.isStringValue(comparison_string)) 
{
> This seems easy to miss that this "is" test method has the side-effect of c
I totally agree with you but I'd rather keep it like this if we ever hope our 
customers take ownership since they use this naming convention.


Line 369:             if (comparison_string.equalsString("<")) {
> Seems a good candidate for a string-switch.
Done


Line 422:                 // For this element of the list, make sure it is 
> trailing ws
Done


Line 581:         format = format.replaceAll("%m", "MM");
> You don't want replaceAll here, that is denoting a regex, I think you're tr
Done


Line 620:                 if (name.equalsIgnoreCase("getyear")) {
> Same comment as above, I would favor string switch on case-normalized name
Done


Line 646:                 if (name.equalsIgnoreCase("getyear") || 
name.equalsIgnoreCase("getmonth")
> Same comment as above, I would favor string switch on case-normalized name
Done


Line 878:             // only one argument 
> trailing ws
Done


Line 1192:                 // use the printpretty on arg0 to spew out 
> trailing ws
Done


Line 1288:                 // absTime with no arguments returns the current 
time. 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/CaseInsensitiveString.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/CaseInsensitiveString.java:

Line 47:         return 0;
> Shouldn't we implement this, comparing the normalized (lower case) value?
huh!!
they didn't have a test case for it.

Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/ClassAd.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/ClassAd.java:

Line 466:         // only try to process if the string is valid 
> trailing ws
Done


Line 499:                 // if caching is enabled, and we got to here then we 
know that the 
> trailing ws
Done


Line 617:                     return EvalResult.EVAL_FAIL.ordinal(); // NAC 
> trailing ws
Done


Line 639:     // --- begin deletion methods 
> trailing ws
Done


Line 717:         // already set by base class for this node; we shouldn't 
propagate 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/ClassAdUnParser.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/ClassAdUnParser.java:

Line 53:      * 
> trailing ws
Done


Line 133:                     // digits as possible, which is why we don't use 
the 
> trailing ws
Done


Line 199:      * 
> trailing ws
Done


Line 473:      * based on the character content, 
> trailing ws
Done


Line 474:      * it's unparsed either as a quoted attribute or non-quoted 
attribute 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/ExprTree.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/ExprTree.java:

Line 38:         /// Attribute reference node (attr, .attr, expr.attr) 
> trailing ws
Done


Line 42:         /// Function call node 
> trailing ws
Done


Line 44:         /// ClassAd node 
> trailing ws
Done


Line 46:         /// Expression list node 
> trailing ws
Done


Line 130:      * 
> trailing ws
Done


Line 139:      * 
> trailing ws
Done


Line 148:      * 
> trailing ws
Done


Line 301:      * 
> trailing ws
Done


Line 320:      * 
> trailing ws
Done


Line 327:      * 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/FunctionCall.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/FunctionCall.java:

Line 108:         // pattern matching (regular expressions) 
> trailing ws
Done


Line 120:         // turn the contents of an expression into a string 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/InputStreamLexerSource.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/InputStreamLexerSource.java:

Line 77:             System.arraycopy(buffer, 0, buffer, 1, buffer.length - 1);
> Doesn't this instead duplicate the next character instead of reinstating th
Not sure how this slipped. But then again, it has been a few months since I 
last looked at this. Not sure if I broke it nowDone


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/Lexer.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/Lexer.java:

Line 580:     // tokenizeStringLiteral:  Scans strings of the form " ... " or 
'...' 
> trailing ws
Done


Line 642:     // tokenizePunctOperator:  Tokenize puncutation and operators 
> trailing ws
Done


Line 840:         // cut the token and return 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/LexerSource.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/LexerSource.java:

Line 31:      * LexerSource that provide access to specific types of sources. 
> trailing ws
Done


Line 53:     // ever put back a single character. 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/Literal.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/Literal.java:

Line 115:     /* Creates an absolute time literal, from the string timestr, 
> trailing ws
Done


Line 215:     /* Creates a relative time literal, from the string timestr, 
> trailing ws
Done


Line 319:     /* Function which iterates through the string Str from the 
location 'index', 
> trailing ws
Done


Line 320:      *returning the index of the next digit-char 
> trailing ws
Done


Line 366:      *  which is the number of seconds since the epoch 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/Operation.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/Operation.java:

Line 41:     /// List of supported operators 
> trailing ws
Done


Line 110:      * 
> trailing ws
Done


Line 133:      * 
> trailing ws
Done


Line 147:      * 
> trailing ws
Done


Line 163:      * 
> trailing ws
Done


Line 674:         // if op is binary, but not associative or commutative, 
disallow splitting 
> trailing ws
Done


Line 927:                     // comparison between strings and non-exceptional 
non-string 
> trailing ws
Done


Line 1218:         // trap sigfpe and set the ClassAdExprFPE flag to true; on 
NT check the 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/Util.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/Util.java:

Line 74:                                 char digit = text.charAt(source + 1); 
// is the next digit also 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/Value.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/Value.java:

Line 85:     public boolean isBooleanValue(MutableBoolean b) {
> Same comment as earlier- personally I would prefer some method naming that 
I too prefer that but I prefer to stick to the users' conventions in hope of 
moving ownership one day :)


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/object/pool/CharArrayStringPool.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/object/pool/CharArrayStringPool.java:

Line 27:         return new AMutableCharArrayString(32);
> Why 32 for pool instances?
arbitrary choice that made some sense. like it is not too small so we will 
re-allocate and not so large so we end up consuming too much memory


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdParserTest.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdParserTest.java:

Line 51:      * 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdToADMTest.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdToADMTest.java:

Line 60:      * 
> trailing ws
Done


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdUnitTester.java
File 
asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdUnitTester.java:

Line 85:             // Then we parse to see what the user wants. 
> trailing ws
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd1fccd136fa2645b2707bbf7c04e60991ae8d4a
Gerrit-PatchSet: 12
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <[email protected]>
Gerrit-Reviewer: Ian Maxon <[email protected]>
Gerrit-Reviewer: Jenkins <[email protected]>
Gerrit-Reviewer: Michael Blow <[email protected]>
Gerrit-Reviewer: Murtadha Hubail <[email protected]>
Gerrit-Reviewer: Till Westmann <[email protected]>
Gerrit-Reviewer: abdullah alamoudi <[email protected]>
Gerrit-HasComments: Yes

Reply via email to