asterixdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <do-not-re...@asterix-gerrit.ics.uci.edu>
Subject Change in asterixdb[master]: This change includes the following things: 1. Allow pregelix...
Date Thu, 23 Jul 2015 03:53:48 GMT
Yingyi Bu has posted comments on this change.

Change subject: This change includes the following things: 1. Allow pregelix statement to
cross two different dataverses. 2. Change TestsUtils to TestsExecutor which is not based on
static methods so that Pregelix can reuse its functionalities. 3. Adds ioDeviceId into th
......................................................................


Patch Set 3:

(7 comments)

https://asterix-gerrit.ics.uci.edu/#/c/325/3/asterix-app/src/main/java/edu/uci/ics/asterix/api/common/AsterixHyracksIntegrationUtil.java
File asterix-app/src/main/java/edu/uci/ics/asterix/api/common/AsterixHyracksIntegrationUtil.java:

Line 63:         File dir = new File(System.getProperty("java.io.tmpdir") + File.separator
+ "nc1");
> This "java.io.tmpdir" is used multiple times, can we make is as a static fi
Done


Line 65:             FileUtils.forceDelete(dir);
> What kind of problem will appear if we didn't delete?
It is to delete test storage files (LSM etc.) in case there are aborted tests before.


https://asterix-gerrit.ics.uci.edu/#/c/325/3/asterix-app/src/main/java/edu/uci/ics/asterix/api/http/servlet/ConnectorAPIServlet.java
File asterix-app/src/main/java/edu/uci/ics/asterix/api/http/servlet/ConnectorAPIServlet.java:

Line 111:             // Flush the cached contents of the dataset to file system.
> Why do we need to flush at this time? Especially in the "doGet" method, do 
External runtimes like Pregelix use this GET method to get the description of the dataset
and read the dataset by themselves.

In case part of the requested dataset is not flushed, e.g., having in-memory LSM components,
 we need to flush (not force) it to make sure the data goes to the OS file system so that
other runtimes can see the data.

For read, we need this "flush" to get the up-to-date data.


https://asterix-gerrit.ics.uci.edu/#/c/325/3/asterix-app/src/main/java/edu/uci/ics/asterix/aql/translator/AqlTranslator.java
File asterix-app/src/main/java/edu/uci/ics/asterix/aql/translator/AqlTranslator.java:

Line 2829:             if (resultState != 1) {
> why do we take the 1 as the succeed? Should it be 0 by the bash convention?
Done


Line 2926:         pr.waitFor();
> Should we catch the `pr.exitValue()` as well? In case some other system err
Done


Line 2972:             if (s.equals("-cust-prop")) {
> just curiously, what's this "-cust-prop" meaning?
it is an option in Pregelix--customized properties.  
A user can put a sequence of comma-separated key=value pairs for that parameter.  Those properties
will be written to the job's configuration XML files as key-value pairs.


https://asterix-gerrit.ics.uci.edu/#/c/325/3/asterix-app/src/test/java/edu/uci/ics/asterix/aql/translator/AqlTranslatorTest.java
File asterix-app/src/test/java/edu/uci/ics/asterix/aql/translator/AqlTranslatorTest.java:

Line 78:                 .asList(new String[] { "bin/pregelix examples/pregelix-example-0.2.13-jar-with-dependencies.jar
"
> Should we make this 0.2.13 be pluggable in order to not break the test once
It could be any xyz.jar at line 68.  The expected result only depends on line 68 but has nothing
to do with the current Pregelix version.
I removed the version number anyway.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7d167b64bf9ec754182b5b2fe44dfc7e5908c686
Gerrit-PatchSet: 3
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Jianfeng Jia <jianfeng.jia@gmail.com>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message