DataOutputAgent limits events after ordering #1444

knu · 2016-04-20T10:53:54Z

This addresses #1044.

cantino · 2016-04-23T17:45:37Z

If a new Agent is linked to this DataOutputAgent, it's older events won't show up because of memory[:last_event_id], right? I guess that's fine. I'm concerned this might be adding a lot of complexity, but it's also probably better than what we have?

brianpetro · 2016-04-23T19:43:09Z

What if we decoupled the data output agent from the prior agent's events?
For example, store all of the required data in an event created by the data
output agent.

On Sat, Apr 23, 2016, 1:45 PM Andrew Cantino notifications@github.com
wrote:

If a new Agent is linked to this DataOutputAgent, it's older events won't
show up because of memory[:last_event_id], right? I guess that's fine.
I'm concerned this might be adding a lot of complexity, but it's also
probably better than what we have?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1444 (comment)

knu · 2016-04-25T05:24:44Z

@cantino That's correct. You could change events_order or events_to_show to reset last_event_id, though.

knu · 2016-04-25T06:08:04Z

@brianpetro Could you elaborate? Is it about making DataOutputAgent create events?

brianpetro · 2016-04-25T10:35:41Z

@knu creating events or even using memory.

The primary reason I say this is because I occasionally have to delete data that is showing up in the data output agent. It probably wouldn't be clear to a newbie that to do this you must dig into the events of the prior agent.

Something like this might enable more sorting strategies too, but I haven't thought that through.

knu · 2016-04-25T11:58:09Z

I'm not sure if it's related to the original problem. Could be another issue?

brianpetro · 2016-04-25T13:54:15Z

@knu probably. I'm still getting a hang of where to add my $0.02

This should fix #1044.

In the first run, it now checks `2 * events_to_show` events for each source. This also fixes the problem where older events are selected when `events_order` is not specified by sorting events by the `id`.

`events_order` determines how to select events for outputting, whereas `events_list_order` determines how to list selected events in the output.

knu · 2016-06-07T06:23:04Z

I renamed events_order to events_list_order with a migration, and re-added events_order that works as it should. I'm not sure if events_list_order is really useful, but the old behavior has been there for awhile and we certainly do not want to break existing agents.

knu · 2016-06-08T07:04:35Z

To explain the implementation, DataOutputAgent keeps a pool of events with the highest scores defined by events_order. Newly received events are compared with those and the pool is updated as necessary. If either events_order is changed or events_to_show is raised, the pool is discarded and recreated it by re-evaluating the most recent 2 * events_to_show events per source (this is new in the revised commits).

cantino · 2016-06-10T06:40:35Z

app/models/agents/data_output_agent.rb

+
+        unless new_events.empty?
+          memory[:last_event_id] = new_events.last.id
+          events.concat(new_events)


Is it possible for there to be duplicates in here from new_events + events, or is new_events guaranteed not to contain any of events?

Yes, the latter, exactly. This is the only place memory[:last_event_id] is set to a non-nil value, and it will be greater than or equal to any value in memory[:event_ids]. new_events are selected by id > memory[:last_event_id] (see above) if memory[:last_event_id] is non-nil, so there will be no duplicates between events and new_events.
If memory[:last_event_id] is nil, memory[:event_ids] is assured to be nil or empty and therefore events is empty, so there will be no duplicates.

I meant it is guaranteed there'd be no duplicates between new_events and events.

cantino · 2016-07-26T01:28:21Z

app/models/agents/data_output_agent.rb

+        DataOutputAgent will select the last `events_to_show` entries of its received events sorted in the order specified by `events_order`, which is defaulted to the event creation time.
+        So, if you have multiple source agents that may create many events in a run, you may want to either increase `events_to_show` to have a larger "window", or specify the `events_order` option to an appropriate value (like `date_published`) so events from various sources are properly mixed in the resulted feed.
+
+        There is also an option `events_list_order` to control the order of events listed in the output, with the same format as `events_order`.  It is defaulted to `#{Utils.jsonify(DEFAULT_EVENTS_ORDER['events_list_order'])}` so the latest entry is listed first.


Maybe "a legacy option events_list_order", or do you think people will keep using it?

Or

"There is also an option events_list_order that only controls the order of events listed in the output, without attempting to maintain a total order of received events. It has the same format as events_order."

I'm having trouble verbalizing the difference between these two subtly-different behaviors.

Great! I've updated the paragraph.

cantino · 2016-07-26T02:27:00Z

When I do an events_order of [["{{title}}"]], I seem to get it in reverse order, with z first. When this is set to events_list_order, it puts A first correctly.

knu · 2016-07-26T11:59:49Z

Yes, that's how it works. An events_order of [["{{title}}"]] means "sort by title in ascending order", so the last N (events_to_show) events with the lexically greatest titles will be selected for output in ascending order, like A, B, … Z. Then the default value of events_list_order ([["{{_index_}}", "number", true]]) means "reverse" in one word, so the final output will look like Z, Y, … A.
I can imagine in most use cases events_order will be based on one of the timestamps each event has, and most feeds and timeline data are presented in reverse chronological order, so I'd consider the default value to be reasonable. The problem is how we document it.

cantino · 2016-08-06T15:38:50Z

Ah, that makes sense. I agree that highest ID or highest date should be first in a feed. Do you have any concerns about merging this now, such as other testing or feedback we should get?

knu · 2016-08-08T05:12:07Z

@cantino At least simple use cases are covered by the specs and the current implementation is kind of broken for the reporters anyway, so I'm going to merge this now and wait for their feedback.

cantino · 2016-08-08T15:37:21Z

👏

knu · 2016-10-07T03:38:38Z

I found a bug in this implementation where I assumed received_events were ordered by id asc and old events would be chosen as a result. I'll come up with a fix shortly.

knu · 2016-10-07T10:27:21Z

Should be fixed in 12cecb8.

knu mentioned this pull request Apr 20, 2016

How to limit number of events from Website Agent? #1044

Closed

knu added 4 commits June 6, 2016 17:41

DataOutputAgent should limit events after ordering

fe50730

This should fix #1044.

Improve the way DataOutputAgent digs past events

9b96a6f

In the first run, it now checks `2 * events_to_show` events for each source. This also fixes the problem where older events are selected when `events_order` is not specified by sorting events by the `id`.

Allow SortableEvents to use keys other than events_order

adab96b

Rename events_order to events_list_order and readd events_order

978ec06

`events_order` determines how to select events for outputting, whereas `events_list_order` determines how to list selected events in the output.

knu force-pushed the data_output_agent_limits_events_after_ordering branch from b298612 to 978ec06 Compare June 7, 2016 06:13

Update RssAgent#events_order

63c35c0

cantino reviewed Jun 10, 2016
View reviewed changes

Remove an unnecessary call to save()

cc8e1bb

cantino reviewed Jul 26, 2016
View reviewed changes

Rephrase the description of events_list_order as suggested by @cantino

26a11af

knu merged commit 9b23c28 into master Aug 8, 2016

knu deleted the data_output_agent_limits_events_after_ordering branch August 8, 2016 05:28

knu restored the data_output_agent_limits_events_after_ordering branch October 7, 2016 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DataOutputAgent limits events after ordering #1444

DataOutputAgent limits events after ordering #1444

knu commented Apr 20, 2016

cantino commented Apr 23, 2016

brianpetro commented Apr 23, 2016

knu commented Apr 25, 2016

knu commented Apr 25, 2016

brianpetro commented Apr 25, 2016

knu commented Apr 25, 2016

brianpetro commented Apr 25, 2016

knu commented Jun 7, 2016 •

edited

Loading

knu commented Jun 8, 2016 •

edited

Loading

cantino Jun 10, 2016

knu Jun 10, 2016 •

edited

Loading

knu Jul 25, 2016

cantino Jul 26, 2016

cantino Jul 26, 2016

knu Jul 26, 2016

cantino commented Jul 26, 2016 •

edited

Loading

knu commented Jul 26, 2016

cantino commented Aug 6, 2016

knu commented Aug 8, 2016

cantino commented Aug 8, 2016

knu commented Oct 7, 2016

knu commented Oct 7, 2016

DataOutputAgent limits events after ordering #1444

DataOutputAgent limits events after ordering #1444

Conversation

knu commented Apr 20, 2016

cantino commented Apr 23, 2016

brianpetro commented Apr 23, 2016

knu commented Apr 25, 2016

knu commented Apr 25, 2016

brianpetro commented Apr 25, 2016

knu commented Apr 25, 2016

brianpetro commented Apr 25, 2016

knu commented Jun 7, 2016 • edited Loading

knu commented Jun 8, 2016 • edited Loading

cantino Jun 10, 2016

Choose a reason for hiding this comment

knu Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

knu Jul 25, 2016

Choose a reason for hiding this comment

cantino Jul 26, 2016

Choose a reason for hiding this comment

cantino Jul 26, 2016

Choose a reason for hiding this comment

knu Jul 26, 2016

Choose a reason for hiding this comment

cantino commented Jul 26, 2016 • edited Loading

knu commented Jul 26, 2016

cantino commented Aug 6, 2016

knu commented Aug 8, 2016

cantino commented Aug 8, 2016

knu commented Oct 7, 2016

knu commented Oct 7, 2016

knu commented Jun 7, 2016 •

edited

Loading

knu commented Jun 8, 2016 •

edited

Loading

knu Jun 10, 2016 •

edited

Loading

cantino commented Jul 26, 2016 •

edited

Loading