[jira] [Comment Edited] (FALCON-87) Hive table integration with feed entity

["Srikanth Sundarrajan (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/FALCON-87?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13755483#comment-13755483 ] 

Srikanth Sundarrajan edited comment on FALCON-87 at 8/31/13 11:20 AM:
----------------------------------------------------------------------

Current user who is creating the feed is being passed as the user.name. I am assuming that this user has enough permission to access the ddl. Please confirm.
{code}
+        ClientResponse response = service.path("ddl/database/").path(database)
+                .path("/table").path(tableName)
+                .queryParam("user.name", CurrentUser.getUser())
+                .accept(MediaType.APPLICATION_JSON)
+                .get(ClientResponse.class);
{code}

Since the catalog service should be created by the factory only if the scheme is catalog, this should never occur, if we check, it might be better throw an illegal state or assertion error. Does it simplify this function if we create a regex pattern and validate the uri against that (and possibly enforce it in xsd as well)?
{code}
+        if (!"catalog".equals(tableUri.getScheme())) {
+            throw new URISyntaxException(tableUri.toString(), "catalog scheme is missing");
+        }
{code}

Should this be equals() instead?
{code}
+    @Override
+    public boolean isIdentical(Storage toCompareAgainst) throws FalconException {
+        CatalogStorage catalogStorage = (CatalogStorage) toCompareAgainst;
+
+        return !(getCatalogUrl() != null && !getCatalogUrl().equals(catalogStorage.getCatalogUrl()))
+                && getDatabase().equals(catalogStorage.getDatabase())
+                && getTable().equals(catalogStorage.getTable())
+                && getPartitions().equals(catalogStorage.getPartitions());
+    }
{code}

junit - reintroduced.
{code}
+import junit.framework.Assert;
{code}

OozieProcessMapper.java::
if the storage is catalog, shouldn't syncdataset uri be a catalog uri in Oozie, if so, prefixing ${nameNode} may be incorrect. May be this was intended to be handled in a separate jira, in which case, it might be better to put a assertion in the process mapper, feed mappers if the scheme is catalog, throw UnsupportedOperationException and the actual handling of these can be taken care of other child tasks of FALCON-85.

{code}
+        syncdataset.setUriTemplate(
+                new Path(locPath).toUri().getScheme() != null ? locPath : "${nameNode}" + locPath);
{code}

Besides, few commented out stale code exists.

                
      was (Author: sriksun):
    Current user who is creating the feed is being passed as the user.name. I am assuming that this user has enough permission to access the ddl. Please confirm.
{code}
+        ClientResponse response = service.path("ddl/database/").path(database)
+                .path("/table").path(tableName)
+                .queryParam("user.name", CurrentUser.getUser())
+                .accept(MediaType.APPLICATION_JSON)
+                .get(ClientResponse.class);
{code}

Since the catalog service should be created by the factory only if the scheme is catalog, this should never occur, if we check, it might be better throw an illegal state or assertion error. Does it simply this function if we create a regex pattern and validate the uri against that (and possibly enforce it in xsd as well)?
{code}
+        if (!"catalog".equals(tableUri.getScheme())) {
+            throw new URISyntaxException(tableUri.toString(), "catalog scheme is missing");
+        }
{code}

Should this be equals() instead?
{code}
+    @Override
+    public boolean isIdentical(Storage toCompareAgainst) throws FalconException {
+        CatalogStorage catalogStorage = (CatalogStorage) toCompareAgainst;
+
+        return !(getCatalogUrl() != null && !getCatalogUrl().equals(catalogStorage.getCatalogUrl()))
+                && getDatabase().equals(catalogStorage.getDatabase())
+                && getTable().equals(catalogStorage.getTable())
+                && getPartitions().equals(catalogStorage.getPartitions());
+    }
{code}

junit - reintroduced.
{code}
+import junit.framework.Assert;
{code}

OozieProcessMapper.java::
if the storage is catalog, shouldn't syncdataset uri be a catalog uri in Oozie, if so, prefixing ${nameNode} may be incorrect. May be this was intended to be handled in a separate jira, in which case, it might be better to put a assertion in the process mapper, feed mappers if the scheme is catalog, throw UnsupportedOperationException and the actual handling of these can be taken care of other child tasks of FALCON-85.

{code}
+        syncdataset.setUriTemplate(
+                new Path(locPath).toUri().getScheme() != null ? locPath : "${nameNode}" + locPath);
{code}

Besides, few commented out stale code exists.

                  
> Hive table integration with feed entity
> ---------------------------------------
>
>                 Key: FALCON-87
>                 URL: https://issues.apache.org/jira/browse/FALCON-87
>             Project: Falcon
>          Issue Type: Sub-task
>    Affects Versions: 0.4
>            Reporter: Venkatesh Seetharam
>            Assignee: Venkatesh Seetharam
>         Attachments: FALCON-87.patch, FALCON-87-review.patch
>
>


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira