Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

simplify batch classes, do not compute JE empty batches, refactor RE worker loop #11737

Merged
merged 1 commit into from
Apr 2, 2020
Merged

simplify batch classes, do not compute JE empty batches, refactor RE worker loop #11737

merged 1 commit into from
Apr 2, 2020

Conversation

colinsurprenant
Copy link
Contributor

@colinsurprenant colinsurprenant commented Mar 31, 2020 •
edited
Loading

This is a followup to #11710 with some cleanups.

  • cleanup RubyArray "rawtypes" and "unchecked" @SuppressWarnings
  • simplify AckedReadBatch
  • change CompiledExecution#compute methods signature to use a Collection instead of RubyArray.
  • optimize/eliminate conversions to RubyArray in a few places.
  • avoid processing empty batches in WorkerLoop which happens when the queue read reaches the timeout.
  • remove QueueBatch#merge method
  • change LinkedHashSet to ArrayList in MemoryReadBatch and AckedReadBatch
  • refactor Ruby pipeline to use same strategy as Java pipeline which does not require merge

@colinsurprenant colinsurprenant changed the title [WIP] simplify batch simplify batch and avoid processing empty batches Mar 31, 2020
@colinsurprenant colinsurprenant marked this pull request as ready for review March 31, 2020 22:25
@elasticsearch-bot elasticsearch-bot self-assigned this Mar 31, 2020
This was referenced Mar 31, 2020
Closed
Closed
@andsel andsel assigned andsel and unassigned elasticsearch-bot Apr 1, 2020
@andsel andsel self-requested a review April 1, 2020 06:57
andsel
andsel reviewed Apr 1, 2020
View reviewed changes
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactoring Colin it's nice to see more typed data structures and less array manipulations, I've left a couple of notes

logstash-core/src/main/java/org/logstash/ackedqueue/AckedBatch.java Outdated Show resolved Hide resolved
logstash-core/src/main/java/org/logstash/ackedqueue/AckedReadBatch.java Outdated Show resolved Hide resolved
@colinsurprenant
Copy link
Contributor Author

colinsurprenant commented Apr 2, 2020 •
edited
Loading

Ok, bare with me here - I managed to make a bit more changes than I originally intended. Basically the last set of changes have to do with the removing of the QueueBatch#merge method because it ended up that this whole LinkedHashSet thing was needed only to support that merge method and only used in the Ruby pipeline which in fact did not really need it. I refactored the Ruby pipeline somewhat similar to the Java pipeline which did not need to merge events. Now everything is more consistent in that respect and also cleaner.

@andsel
Copy link
Contributor

andsel commented Apr 2, 2020

Globally LGTM there is only another location where merge it's used and make the Ruby's unit tests fail

@colinsurprenant
Copy link
Contributor Author

@andsel Yikes - Missed that one. Fixed!

@andsel andsel self-requested a review April 2, 2020 14:59
@colinsurprenant
simplify batch classes, do not compute JE empty batches, refactor RE …
4a06ea3 
…worker loop

cleanup RubyArray "rawtypes"
remove all LinkedHashSet from batch and queue classes
avoid processing empty batches in Java worker loop
cleanup AckedReadBatch and MemoryReadBatch
refactor Ruby worker loop similar to Java Execution to not use batch merge
remove QueueBatch merge and replace LinkedHashSet with ArrayList
@colinsurprenant colinsurprenant changed the title simplify batch and avoid processing empty batches simplify batch classes, do not compute JE empty batches, refactor RE worker loop Apr 2, 2020
@colinsurprenant colinsurprenant mentioned this pull request Apr 2, 2020
Merged
@colinsurprenant colinsurprenant mentioned this pull request Apr 2, 2020
Merged
@colinsurprenant colinsurprenant merged commit 5a25c6f into elastic:master Apr 2, 2020
colinsurprenant added a commit that referenced this pull request Apr 2, 2020
@colinsurprenant
simplify batch classes, do not compute JE empty batches, refactor RE …
f4b9349 
…worker loop (#11746)

7.x clean backport or #11737

cleanup RubyArray "rawtypes"
remove all LinkedHashSet from batch and queue classes
avoid processing empty batches in Java worker loop
cleanup AckedReadBatch and MemoryReadBatch
refactor Ruby worker loop similar to Java Execution to not use batch merge
remove QueueBatch merge and replace LinkedHashSet with ArrayList
robbavey added a commit to robbavey/logstash-devutils that referenced this pull request Apr 23, 2020
@robbavey
Add events method to QueueBatchDelegator
301581b 
After elastic/logstash#11737 was committed, plugin unit tests
using `sample` targeted against branches with this change merged were failing due to the lack of
`events` method on the QueueBatchDelegator.

This commit adds an `events` method to fix the failing tests.
@robbavey robbavey mentioned this pull request Apr 23, 2020
Merged
@colinsurprenant
Copy link
Contributor Author

This was merged in master with 5a25c6f - closing.

@colinsurprenant colinsurprenant deleted the batch_cleanups branch May 21, 2020 13:56
@luizgpsantos luizgpsantos mentioned this pull request Jun 1, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andsel andsel andsel approved these changes

Assignees

@andsel andsel

Labels
code cleanup Java Execution v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@colinsurprenant @andsel @elasticsearch-bot