Revert PR #1218

[Nick Allen <ni...@nickallen.org>]

Hi Guys -

@rmerriman tracked down some problems that were introduced with my PR
#1218.  Thanks to him for finding this.  The change was intended to improve
Elasticsearch write performance by allowing Elasticsearch to set its own
document ID.

The problem is that if you then go to the Alerts UI and escalate an alert,
it will create a duplicate alert in the index, rather than updating the
existing alert. I've been looking at how to fix the problem and the scope
of the fix is larger than I'd like to handle as a follow-on.  There are
some prerequisites I'd like to tackle before introducing this change.

I am going to revert the change on master, which will introduce an
additional commit that is an "undo" of the original commit.  I will then
open a separate PR that introduces this new functionality.

https://github.com/apache/metron/pull/1218

Thanks

Re: Revert PR #1218

[Nick Allen <ni...@nickallen.org>]

I wouldn't call this complex.  It is much easier to roll it back, so I can
work on a proper fix without impacting the ongoing work of others.

The existing Elasticsearch DAOs do not distinguish between document ID and
Metron GUID as there was no need to before.  So I need to disambiguate
those concepts a bit, which is rather subtle.  In addition, none of the
integration or e2e tests caught the problem because there is a disconnect
between the reader and writer side of the house for Elasticsearch.  I want
to update the tests to ensure this sort of problem is caught.


On Tue, Oct 23, 2018 at 3:11 PM Simon Elliston Ball <
simon@simonellistonball.com> wrote:

> Would it not make more sense to fix the bug on the DAO side, and roll
> forward? I suspect what we need to do is add a stage in the update
> capability to configure the key field used for update, or worst case have a
> pre-query to lookup the internal ID in the relatively rare scenario where
> we escalate / modify indexed docs. Seems like a simple new ticket, rather
> than a complex roll back and roll forward later. As long as we get the
> follow on in before an Apache release we should be fine, no?
>
> Simon
>
> On Tue, 23 Oct 2018 at 19:58, Nick Allen <ni...@nickallen.org> wrote:
>
> > Hi Guys -
> >
> > @rmerriman tracked down some problems that were introduced with my PR
> > #1218.  Thanks to him for finding this.  The change was intended to
> improve
> > Elasticsearch write performance by allowing Elasticsearch to set its own
> > document ID.
> >
> > The problem is that if you then go to the Alerts UI and escalate an
> alert,
> > it will create a duplicate alert in the index, rather than updating the
> > existing alert. I've been looking at how to fix the problem and the scope
> > of the fix is larger than I'd like to handle as a follow-on.  There are
> > some prerequisites I'd like to tackle before introducing this change.
> >
> > I am going to revert the change on master, which will introduce an
> > additional commit that is an "undo" of the original commit.  I will then
> > open a separate PR that introduces this new functionality.
> >
> > https://github.com/apache/metron/pull/1218
> >
> > Thanks
> >
>
>
> --
> --
> simon elliston ball
> @sireb
>

Re: Revert PR #1218

[Simon Elliston Ball <si...@simonellistonball.com>]

Would it not make more sense to fix the bug on the DAO side, and roll
forward? I suspect what we need to do is add a stage in the update
capability to configure the key field used for update, or worst case have a
pre-query to lookup the internal ID in the relatively rare scenario where
we escalate / modify indexed docs. Seems like a simple new ticket, rather
than a complex roll back and roll forward later. As long as we get the
follow on in before an Apache release we should be fine, no?

Simon

On Tue, 23 Oct 2018 at 19:58, Nick Allen <ni...@nickallen.org> wrote:

> Hi Guys -
>
> @rmerriman tracked down some problems that were introduced with my PR
> #1218.  Thanks to him for finding this.  The change was intended to improve
> Elasticsearch write performance by allowing Elasticsearch to set its own
> document ID.
>
> The problem is that if you then go to the Alerts UI and escalate an alert,
> it will create a duplicate alert in the index, rather than updating the
> existing alert. I've been looking at how to fix the problem and the scope
> of the fix is larger than I'd like to handle as a follow-on.  There are
> some prerequisites I'd like to tackle before introducing this change.
>
> I am going to revert the change on master, which will introduce an
> additional commit that is an "undo" of the original commit.  I will then
> open a separate PR that introduces this new functionality.
>
> https://github.com/apache/metron/pull/1218
>
> Thanks
>


-- 
--
simon elliston ball
@sireb