[
https://issues.apache.org/jira/browse/METRON-1421?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16410335#comment-16410335
]
ASF GitHub Bot commented on METRON-1421:
----------------------------------------
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 thing I was trying to fix. I
feel like we are running into the same problem.
I apologize if this feedback is too abstract and not specific enough.
Maybe I'm way off base because I haven't spent enough time in this portion of
the code. Maybe some of this is pre-existing, but whatever the case is, I'd
really like to try and avoid repeating another #832.
(1) Use composition over inhertence
I think trying to share code between the two implementations (ES and Solr)
via inheritance with an abstract base class is going to get us in trouble. I
would do exactly the kind of refactoring you did, but instead via composition.
The two implementations are now fairly tied at the hip. Code is shared
between the two in the base class without well-defined abstractions defining
how they interact. Changes in the base class to evolve Solr is likely to break
ES or vice versa.
A good example of this is in `removeAlertsFromMetaAlert` lines 128 - 131.
It seems we had to add some logic in here due to a qwerk in Solr. We are only
going to find more qwerks over time. The abstraction is already breaking a bit
here and will continue to do so.
(2) These DAOs are big.
I don't see that many unit tests around this; mostly integration tests.
And I think that is partly because we could break this logic down into
multiple, smaller chunks to make testing it easier. And that also makes it
easier for newbies to grok.
For example, there is logic in there for calculating meta scores. That
should be broken out into its own class whose sole responsibility is to do
that. Then we can easily write unit tests around it. Right now, that logic is
all tied up in the abstract base class which prevents us from easily unit
testing it.
There is other high-level logic like that, that we could easily test in
unit tests, if we refactored this and broke this down into smaller chunks.
* Do not add/remove alerts from/to a metaalert that is not active.
* Only add alert to a meta alert, if it does not already exist
* Only remove alert it it is actually in the metaalert
* If making meta alert active, add the meta alert guid to every attached
alert.
> Create a SolrMetaAlertDao
> -------------------------
>
> Key: METRON-1421
> URL: https://issues.apache.org/jira/browse/METRON-1421
> Project: Metron
> Issue Type: Sub-task
> Reporter: Justin Leet
> Assignee: Justin Leet
> Priority: Major
>
> Create an implementation of the MetaAlertDao for Solr. This will involve
> implementing the various MetaAlertDao methods using the SolrJ library and
> also providing a SolrMetaAlertIntegrationTest (similar to
> ElasticsearchMetaAlertIntegrationTest).
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
