Re: Review Request 70536: ATLAS-3157: Add Integration tests for Hive metastore hook

2019-04-24 Thread Na Li via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70536/#review214855
---




addons/hive-bridge/src/test/java/org/apache/atlas/hive/HiveITBase.java
Lines 110 (patched)


It is not obvious here why you do it?

Can you add comment that driver using this configuration will be used to 
test HiveHook, so HMS hook is not configurated?



addons/hive-bridge/src/test/java/org/apache/atlas/hive/HiveITBase.java
Line 131 (original), 137 (patched)


add comment



addons/hive-bridge/src/test/java/org/apache/atlas/hive/HiveITBase.java
Line 138 (original), 146 (patched)


to make the logic clear, you can make runCommand(String cmd) as abstract 
function. And the child class implement it.

For example, HiveHookIT implements with driver , and HiveMetastoreHookIT 
implements with driverWithoutContext.


- Na Li


On April 24, 2019, 1:34 a.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70536/
> ---
> 
> (Updated April 24, 2019, 1:34 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, keval bhatt, Na 
> Li, Le Ma, Madhan Neethiraj, Nikhil Bonte, and Nixon Rodrigues.
> 
> 
> Bugs: ATLAS-3157
> https://issues.apache.org/jira/browse/ATLAS-3157
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Add Integration tests for Hive metastore hook
> 
> 
> Diffs
> -
> 
>   addons/hive-bridge/src/test/java/org/apache/atlas/hive/HiveITBase.java 
> c8c53c473 
>   addons/hive-bridge/src/test/java/org/apache/atlas/hive/hook/HiveHookIT.java 
> 677043a01 
>   
> addons/hive-bridge/src/test/java/org/apache/atlas/hive/hook/HiveMetastoreHookIT.java
>  PRE-CREATION 
>   addons/hive-bridge/src/test/resources/hive-site.xml 4605ae322 
> 
> 
> Diff: https://reviews.apache.org/r/70536/diff/1/
> 
> 
> Testing
> ---
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1048/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>

Re: Review Request 70536: ATLAS-3157: Add Integration tests for Hive metastore hook

2019-04-24 Thread Na Li via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70536/#review214853
---




addons/hive-bridge/src/test/java/org/apache/atlas/hive/HiveITBase.java
Lines 349 (patched)


There could be a problem in calling waitFor() to verify that the entity is 
not registered.

Notice in waitFor(), the Predicate is first called. If not exception 
happens, the test returns. If there is not found exception, it waits and try 
again. It is to verify something eventually happens with delay.

To test entity not registered, we need to wait first, then test to avoid 
the situation that
1) query of entity happens first, not found, and test return
2) registration happens later
3) The test does not detact registration actually happens

protected void waitFor(int timeout, Predicate predicate) throws 
Exception {
ParamChecker.notNull(predicate, "predicate");
long mustEnd = System.currentTimeMillis() + timeout;

while (true) {
try {
predicate.evaluate();
return;
} catch(Error | Exception e) {
if (System.currentTimeMillis() >= mustEnd) {
fail("Assertions failed. Failing after waiting for 
timeout " + timeout + " msecs", e);
}
LOG.debug("Waiting up to {} msec as assertion failed", 
mustEnd - System.currentTimeMillis(), e);
Thread.sleep(5000);
}
}
}


- Na Li


On April 24, 2019, 1:34 a.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70536/
> ---
> 
> (Updated April 24, 2019, 1:34 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, keval bhatt, Na 
> Li, Le Ma, Madhan Neethiraj, Nikhil Bonte, and Nixon Rodrigues.
> 
> 
> Bugs: ATLAS-3157
> https://issues.apache.org/jira/browse/ATLAS-3157
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Add Integration tests for Hive metastore hook
> 
> 
> Diffs
> -
> 
>   addons/hive-bridge/src/test/java/org/apache/atlas/hive/HiveITBase.java 
> c8c53c473 
>   addons/hive-bridge/src/test/java/org/apache/atlas/hive/hook/HiveHookIT.java 
> 677043a01 
>   
> addons/hive-bridge/src/test/java/org/apache/atlas/hive/hook/HiveMetastoreHookIT.java
>  PRE-CREATION 
>   addons/hive-bridge/src/test/resources/hive-site.xml 4605ae322 
> 
> 
> Diff: https://reviews.apache.org/r/70536/diff/1/
> 
> 
> Testing
> ---
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1048/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>

Re: Review Request 70536: ATLAS-3157: Add Integration tests for Hive metastore hook

2019-04-24 Thread Madhan Neethiraj 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70536/#review214852
---


Ship it!




Ship It!

- Madhan Neethiraj


On April 24, 2019, 1:34 a.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70536/
> ---
> 
> (Updated April 24, 2019, 1:34 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, keval bhatt, Na 
> Li, Le Ma, Madhan Neethiraj, Nikhil Bonte, and Nixon Rodrigues.
> 
> 
> Bugs: ATLAS-3157
> https://issues.apache.org/jira/browse/ATLAS-3157
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Add Integration tests for Hive metastore hook
> 
> 
> Diffs
> -
> 
>   addons/hive-bridge/src/test/java/org/apache/atlas/hive/HiveITBase.java 
> c8c53c473 
>   addons/hive-bridge/src/test/java/org/apache/atlas/hive/hook/HiveHookIT.java 
> 677043a01 
>   
> addons/hive-bridge/src/test/java/org/apache/atlas/hive/hook/HiveMetastoreHookIT.java
>  PRE-CREATION 
>   addons/hive-bridge/src/test/resources/hive-site.xml 4605ae322 
> 
> 
> Diff: https://reviews.apache.org/r/70536/diff/1/
> 
> 
> Testing
> ---
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1048/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>