METRON-532 Define Profile Period When Calling PROFILE_GET

[mattf-apache]

This PR builds on top of @nickwallen 's work in METRON-529, which has already been committed to master. It adds an optional argument to PROFILE_GET, allowing run-time overrides of the profiler client global configuration parameters. The primary use case is when historical profiles have been created with a different profile configuration than is currently configured, and the analyst needing to access them does not want to change the global client configuration so as not to disrupt the work of other analysts working with current profiles.

This PR is essentially similar to the former proposed PR-345, except that the signature of the PROFILE_GET function has been changed to make groups be a List instead of a "varargs"-style sequence, so the new config_overrides Map argument can come after it. This avoids a really hacky over-indulgence in polymorphism, and is consistent with discussion in PR-395. I'm opening a new PR, rather than reopening the old one, because I also had to rebase to changes in master, and the forced push destroyed the history that PR-345 pointed at.

At suggestion of @cestella , scope of this work item was expanded to include checking for config changes at run time, in the PROFILE_GET Stellar function. This is a much needed enhancement.

I also considerably improved the documentation of PROFILE_GET as regards the groups argument, with input from @nickwallen . Thanks, Nick!

Testing: This build has passed Travis in my fork, and should shortly be reported passed here. Since the only code changes were to the Stellar function, the corresponding junit tests are actually sufficient testing. However, I will go ahead and test them on a live single-host test platform, taking the opportunity to test the new Profiler RPMs from PR-413! I'll report the results here tomorrow.

[cestella]

Ok, I like this a lot. I didn't see anything that I really have an issue with at all, in fact. Great job @mattf-horton ! You get my +1 pending your tests :)

[mmiklavc]

Clean, well-documented, tested. Nice work @mattf-horton! +1 from me as well.

[nickwallen]

The function has two optional arguments, no? I thought per the discussion in METRON-590 that was not well liked? Is this different somehow?

[nickwallen]

Just curious, why switch the comment style here? When should I use javadoc comments versus regular comments?

[mattf-apache]

Um, you got me. Perhaps my IDE "helped" me with this one. Since the member variable being documented is private, there was no real reason to switch to a Javadoc-style comment. The point of the edit was to add the word "cached" to the comment. If I make any other tweaks, I'll change it back to a "//" comment.

There is a rule, which is that Javadoc comments should be used with public interfaces, and it doesn't matter which you use with private interfaces, altho no harm in using the Javadocs format if you want the documentation of a private i/f to be really clear.

[mattf-apache]

@nickwallen , I was mistaken. This IS a private member variable, so the comment can be either format. I'll change it back, and also change the next one (cachedConfigMap) also, since it's the only one using "//" in this module, even for private members.

[nickwallen]

You can leave it however you like. I was just curious for my own future reference.

[nickwallen]

What does setup2 do differently than setup? Could we comment or rename the function to be more descriptive?

[mattf-apache]

This is a test to demonstrate that the "current" configuration can be set differently than the "historical" configuration, and then both the positive and negative test cases work right. setup2() is modeled exactly on setup(), except that it uses periodDuration2, periodUnits2 and saltDivisor2, instead of periodDuration, periodUnits and saltDivisor respectively.

However, you're right that this is a little opaque, because setup() is called in the @before clause, while setup2() is called in the midst of the test routine. Let me add some comments that clarify this.

[nickwallen]

Just curious, why change the comment style? When should I use javadoc-style comments versus traditional //?

[mattf-apache]

As above.

[nickwallen]

Looks really good. Have some minor questions, but that's all.

I just want to make sure everyone is aware of and OK with multiple optional args as this was criticized previously. I personally have no problem with it.

[cestella]

@nickwallen Yeah, I noticed that. I THINK I've come around to it in this case. They are differing types so should be able to be differentiated relatively easily. Thanks for bringing attention to it, though. It won't hurt to ensure nobody else is offended by it.

[nickwallen]

Sounds good. +1

[mattf-apache]

@nickwallen and @cestella, thanks for the comments. Regarding multiple optional args, I followed the rule that if the second optional arg is provided, then the first "optional" arg isn't optional; or more generally, if a later optional arg is provided, all preceding optional args must also be provided, even if with trivial values. No tricky polymorphisms with object types, just a simple argument count. I think that's the best we can do without named arguments.

BTW, @cestella , my previous PR-345 did use object type checking to allow either or both to be truly optional, but it felt very hacky, was obviously not generalizable, and I was happy to change to this.

Hopefully I made this clear for users in both the comments and documents. Let me know if you feel more needs to be said. Thanks.

[mattf-apache]

A further comment on multiple optional parameters: C++ allows multiple optional (defaulted) arguments, and does not support named arguments. The rule "if a later optional arg is provided, all preceding optional args must also be provided, even if with trivial values", is exactly what C++ does. It is also consistent with what Python does when multiple defaulted arguments are provided positionally instead of as "named arguments" -- altho of course in Python one probably would use the named args instead.

[cestella]

@mattf-horton Thanks for pointing out the precedent. It is to my everlasting shame that I did not think to do named arguments initially for Stellar.

[mattf-apache]

@cestella , Everlasting? Really? :-) Considering that Java doesn't, and even most interpreted languages that do support them added them as a 2nd or 3rd generation feature, I'd say you're in very good company!

[cestella]

@mattf-horton hah fair point, I was being overwrought, but we might consider it if we ever have a bundle of Stellar changes to make.

[mattf-apache]

@cestella @mmiklavc @nickwallen , thank you for the review comments. Please see latest for improvements to comments per your suggestions. No code was changed. Also fixed a doc bug regarding profiler "init" syntax in example code.

This code has been validated on a single-node 16GB Centos7 vm with OracleJDK8, HDP-2.5.3, and Ambari-2.4.2. The cluster was installed via my metron-mpack-singlenode, then Profiler was added with @nickwallen 's RPMs. Bro data was pumped through the usual sequence of topologies, and was observed in Storm UI to propagate through all into indexer and profiler. Elasticsearch and HBase were queried from outside Metron to confirm presence and format of data, and PROFILE_GET was used in Stellar REPL to read the profile data back out as expected. Use of the config_overrides map argument in PROFILE_GET was also observed to be effective.

[mattf-apache]

Couple additional fixes to the documentation. Statements about storage of the writer and client configs were incorrect.