[GitHub] metron issue #1012: METRON-1551 Profiler Should Not Use Java Serialization

2018-05-14 Thread nickwallen
Github user nickwallen commented on the issue:

https://github.com/apache/metron/pull/1012
  
Reopen for unrelated failure.  Will open a bug for what seems to be an 
intermittent test failure.
```
Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.032 sec 
<<< FAILURE! - in org.apache.metron.stellar.common.CachingStellarProcessorTest
testCaching(org.apache.metron.stellar.common.CachingStellarProcessorTest)  
Time elapsed: 0.031 sec  <<< FAILURE!
java.lang.AssertionError: expected:<6> but was:<5>
```


---


[GitHub] metron issue #1012: METRON-1551 Profiler Should Not Use Java Serialization

2018-05-14 Thread cestella
Github user cestella commented on the issue:

https://github.com/apache/metron/pull/1012
  
Looks good; +1 by inspection pending travis


---


[GitHub] metron issue #1012: METRON-1551 Profiler Should Not Use Java Serialization

2018-05-14 Thread nickwallen
Github user nickwallen commented on the issue:

https://github.com/apache/metron/pull/1012
  
I made two updates. 

* I added the additional test check to `StellarProcessorUtils` to ensure 
the value returned is Java serializable.  There were a few classes that I had 
to update for the tests to continue to pass.  All simple cases where a class 
just needed to implement Serializable.

* Updated all Profiler classes to be Java serializable in case a user 
chooses to use Java serialization in their Storm topology. This is not 
recommended, but there is no reason to block a user from doing so.






---


[GitHub] metron issue #1012: METRON-1551 Profiler Should Not Use Java Serialization

2018-05-14 Thread cestella
Github user cestella commented on the issue:

https://github.com/apache/metron/pull/1012
  
Ok, this looks great, one request though.  It appears that, at least for 
the profiler, it is required that objects used in Stellar be Java serializable 
if they aren't listed in that exception list.

We ensure that the objects are kryo serializable 
[here](https://github.com/apache/metron/blob/91a017b6dcefc250bfd67cecf9803cb59015d213/metron-stellar/stellar-common/src/main/java/org/apache/metron/stellar/common/utils/StellarProcessorUtils.java#L64)
  Can we also make a blurb that they're java serializable?  I know that 
OnlineStatisticsProvider will not be java serializable due to its dependence on 
t-digest, so maybe make that check optional but on by default?



---