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 asterixdb[master]: Enabled Datasets to use Datatypes from foreign Dataverses
Date Thu, 07 Jan 2016 08:09:37 GMT
abdullah alamoudi has posted comments on this change.

Change subject: Enabled Datasets to use Datatypes from foreign Dataverses
......................................................................


Patch Set 10:

(11 comments)

Hi Steven,
Some small comments. Please have a look. 
My biggest concern is the deletion of the test cases.

In addition, one of the rules for (introduce access methods), it checks if the secondary index
is a metadata index and then skips it if it is. can we remove that check since it doesn't
make sense anymore?

Thanks,
Abdullah.

https://asterix-gerrit.ics.uci.edu/#/c/558/10/asterix-app/src/main/java/org/apache/asterix/file/ExternalIndexingOperations.java
File asterix-app/src/main/java/org/apache/asterix/file/ExternalIndexingOperations.java:

Line 254:      * =======
Conflict was not resolved correctly.
Please, fix.


Line 293:      * <<<<<<< HEAD
Conflict was not resolved correctly. 
please, fix.


https://asterix-gerrit.ics.uci.edu/#/c/558/10/asterix-app/src/test/resources/metadata/queries/basic/meta16/meta16.3.query.aql
File asterix-app/src/test/resources/metadata/queries/basic/meta16/meta16.3.query.aql:

Line 19: /*
why was meta16 test changed instead of creating a new test?


https://asterix-gerrit.ics.uci.edu/#/c/558/10/asterix-app/src/test/resources/metadata/queries/basic/meta17/meta17.3.query.aql
File asterix-app/src/test/resources/metadata/queries/basic/meta17/meta17.3.query.aql:

Line 23:  */
why change this instead of keeping it and creating a new one?


https://asterix-gerrit.ics.uci.edu/#/c/558/10/asterix-app/src/test/resources/metadata/queries/basic/meta18/meta18.1.ddl.aql
File asterix-app/src/test/resources/metadata/queries/basic/meta18/meta18.1.ddl.aql:

Line 17
why delete a test case? does it still pass?


https://asterix-gerrit.ics.uci.edu/#/c/558/10/asterix-app/src/test/resources/metadata/queries/basic/meta19/meta19.1.ddl.aql
File asterix-app/src/test/resources/metadata/queries/basic/meta19/meta19.1.ddl.aql:

Line 17
why deleting this test case too?


https://asterix-gerrit.ics.uci.edu/#/c/558/10/asterix-app/src/test/resources/metadata/queries/basic/meta20/meta20.1.ddl.aql
File asterix-app/src/test/resources/metadata/queries/basic/meta20/meta20.1.ddl.aql:

Line 17
why deleting this test case as well.


https://asterix-gerrit.ics.uci.edu/#/c/558/10/asterix-app/src/test/resources/metadata/queries/basic/meta21/meta21.1.ddl.aql
File asterix-app/src/test/resources/metadata/queries/basic/meta21/meta21.1.ddl.aql:

Line 17
why deleting the test case as well?


https://asterix-gerrit.ics.uci.edu/#/c/558/10/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataNode.java
File asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataNode.java:

Line 731:             throws MetadataException, RemoteException {
Please, add a failing test case for this one.


Line 743:             throws MetadataException, RemoteException {
This method has a bunch of issues:
1. you need to take the subdata type item and break it into its two components (dataverse
name and data type name). then check and make sure that the dataverse of the subtype == the
dataverse of the type to be deleted.

2. the way this is implemented means all the types will be loaded in memory. if the system
was used for a very long time, then the memory cost of this will be too much. I know this
is right now is done like this everywhere. can you please add a todo for this?

3. Did you add test cases to test this failure scenario? do some with fully qualified names
and some without.

4. the error message in the exception is not correct. the second dataverse name should be
the dataverse name of the other data type.


https://asterix-gerrit.ics.uci.edu/#/c/558/10/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java
File asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java:

Line 2055:      * 
white space.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I24dbc04dcb2a4126fc8361ebe3104877a0d1f2bb
Gerrit-PatchSet: 10
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Steven Jacobs <sjaco002@ucr.edu>
Gerrit-Reviewer: Ildar Absalyamov <ildar.absalyamov@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Preston Carman <prestonc@apache.org>
Gerrit-Reviewer: Steven Jacobs <sjaco002@ucr.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message