Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
This is merged into the feature branch. Closing PR manually, accordingly.
---
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/970
+1 from me. Thanks @justinleet, this was a beast.
---
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/970
Given the added fix by @merrimanr, this lgtm. +1
---
Github user cestella commented on the issue:
https://github.com/apache/metron/pull/970
I'm giving this a +1 after going through the test script, great job and
thanks for the effort.
---
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Pulled in https://github.com/justinleet/metron/pull/18 from @merrimanr to
address the reported bugs when metaalerts become empty. Thanks for digging
into the bug you found, it's super helpful!
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/970
@justinleet sounds reasonable to me
---
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr I'll add a couple tests for those and see if I can get them
reproduced. Since some of that is in the AbstractLucene dao I'm wondering if it
also happens against master ES. When I take
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@mmiklavc I'm also inclined to work on that fix against master, since it
effects current ES metaalerts. Seem reasonable?
---
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@mmiklavc
Re: The index issue appears to be preexisting.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/970
I spun this up in full dev and ran through all the tests in the test plan
above. Everything in this plan worked. I also added a step in each test to
perform a search on alert fields and verified
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/970
Just finished testing this in full dev. Everything basically looks good.
I'll take another spin through the source code as well. This is a good test
script @justinleet.
Here are my
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Last commit is the one I tested on. I just forgot to push it up.
---
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Sidenote, after the remove alert section, make sure to add it back if you
want to reuse the same metaalert for next section. I left it out, since you'll
need your own guids for everything.
---
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
I have run the following set of manual tests against the REST UI.
Do the following to setup Solr after ssh'ing into full dev
```
sudo su -
export METRON_HOME=/usr/metron/0.4.3/
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Merged in master, and in particular METRON-1540. Minor updates occurred to
get everything aligned and the columns count was incremented by one for a
couple test cases (for the schemas' metaalert
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
The workaround for the intermittent failure described above was changed.
The workaround didn't produce complete results for children. Instead, I've
moved to a second query that pulls in the
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Merged in a PR from @merrimanr that fixed a couple bugs, in particular
around committing (which would happen in tests but no on full dev)
---
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
I've been looking into the Travis failure that occurred. I was able to
reproduce it, intermittently, locally. Best speculation is that it's a Lucene
bug, given that it occurs seemingly randomly
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr The previous commits should also (hopefully) resolve the issues
with `source.type` vs `source:type`. Given that there were issues with the
tests and test schemas themselves I haven't
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen Refactoring should be done, barring any additional changes.
The only abstract class ends up being the UpdateDao (unless we want to be
consistent), because mutation is really the main
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@mmiklavc I think that's a good insight. The original intention of
metaalerts was to basically wrap and augment the functionality of the index
alerts. I think a portion of that problem we've
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/970
@justinleet @nickwallen I agree with the proposed plan for this PR's
refactorings. That being said, I have some comments after having more or less
caught up with the thread.
Generally
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr I should probably hit you up too before we confirm that's what
the plan is, since you've run stuff up a couple times and been pretty involved
in the Solr work. Does the above plan for
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
I agree with your plan @justinleet . I am on-board. Thanks for laying
that out.
---
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@ottobackwards You're absolutely not confusing things. It's important to
have that perspective, and I am 100% appreciative of you hopping in and
offering up your opinion. I think you're right
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/970
@justinleet @nickwallen , sorry I didn't mean to confuse things. It seems
to me that @nickwallen is willing to do some work on this per his 'vision' and
that might be easier if this landed in
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
Just to summarize my position, my suggested refactorings are just one of
many ways to improve the testability of all this. We don't have to take this
approach.
For this PR, I will be
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@ottobackwards Correct me if I'm misunderstanding your view, @nickwallen,
but the discussion is around what's the required level of iteration /
refactoring where this meets expectations for
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/970
As we are already on a feature branch, can we not iterate and refactor
this? Does it all have to go into *this* pr?
If this is landed, what is keeping @nickwallen from throwing a PR
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen
Assuming we go through with the partial refactoring, the existence of those
interfaces does not prevent us from testing the individual DAOs. A lot of the
highly specific changes
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
Do you think we have enough unit tests on this functionality how it is?
---
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
> I'd like to know what specific problems we're going to solve that merit
that level of change in this PR.
The fundamental problem here is a lack of good unit tests on these search
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen
Right now those interfaces are basically a marker interface that requires
everything that's needed for a complete DAO set. Prior to some of the Solr
refactorings, it existed as a
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
> @justinleet Solve the code reuse problem for Solr and ES.
I am also noticing another positive side effect here. Getting rid of these
interface hierarchies (like having the
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
It is definitely looking better. I am liking the progress. But I still
think having overly broad interfaces that do everything is causing problems;
primarily `IndexDao` and also `MetaAlertDao`.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen I did a super POC refactoring I think is closer to what you're
looking for. It only restructures the code more the way you're talking about
(for Solr only, didn't have time to push
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr I'll have to dig into that more. The stuff at the MultiIndexDao
level wasn't touched, so I really don't know why the sensor type doesn't seem
to be making it down there. It's
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/970
I'm still getting an error in full dev. When I execute this create
metaalert function in REST:
```
{
"alerts": [
{
"guid": "45534065-a8bb-4c01-8c83-1812e661f76e",
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/970
So, @nickwallen , do you propose implementation of Decorator or another
such pattern? Is this problem inherent in the ES part as well? Should the
whole thing be changed? If so in this PR or
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
I dug into this further. I think fundamentally there are a few issues
causing problems.
1. The extensive inheritance amongst all of these interfaces is
problematic. This introduces
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen I made a pass at the refactoring. Most things end up in the
UpdateDao (which is probabyl to be expected, given that mutation is the main
sticking point). Even though it largely ends
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr I fixed the metaalert template; I'd forgotten to update the
original placeholder one in addition to the test one. I haven't had a chance
to run it up and see if it fixes what you saw,
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
> @justinleet Are you more interested into breaking things out into more
classes than that, just breaking apart functions more so they're more easily
tested, some combination of both?
>
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen My main concern re: pluggability is for a lot of this stuff,
it's going to be very pretty specific to the fact that (right now) we're using
Lucene stores (and it might be worth
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr I'll take a look and see where things are falling apart and make
sure to get whatever test case sorted out. Sounds like the branch is in a
state where we just spin up full dev, get
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/970
I spun this up in full dev and can't quite get it to work. In the first
test I am trying to create and modify a metaalert directly through Swagger.
After switching the search engine to Solr and
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
Thanks @justinleet for consuming my rant. :)
I agree with most of what you're saying. I'm perfectly happy iterating
improvements on this. And totally willing to help. One
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen For a bit of context, I tried primarily to keep things
essentially how they were in the ES versions (including the integration tests
and unit tests). I definitely appreciate you
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
This is bringing me flashbacks from when I did #832. I found the Index
DAOs really hard to work with and grok back then. I had to refactor them
slightly to get decent unit tests for the one
49 matches
Mail list logo
