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.

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

LIR Read Only #6708

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@andrewvc
Contributor

andrewvc commented Feb 14, 2017

Overview

This is an Initial merge of LIR code into master to help keep all this stuff up to date. This won't affect any existing execution, but is simply meant to be an incremental step on the way to #6281 , which will come in a separate merge. There is potential for us to find some configs that break LIR, which is why we should just merge this into master for now.

Design Goals

  1. Move the Conceptual Core of Logstash from a Function to a DAG: Currently, logstash pipeline sections are declared in an imperative configuration language and then transpiled to one Ruby function per section. While this strategy has served logstash well throughout its history it is limited. A DAG representation allows for easier manipulation before runtime and easier introspection during runtime.
  2. Enable a Faster Runtime: With a more agnostic DAG representation experimenting with alternate execution methodologies becomes much simpler. Our first stab at this is #6281. Previously, in #4727, I benchmarked a DAG based PoC that showed significant performance improvements for real-world workloads. Additionally, regardless of execution strategy, transpiling to Ruby limits Logstash's performance to Ruby performance on the JVM and excludes experimentation with other approaches.
  3. Open up the Possibility of Alternate Config Languages: Currently Logstash supports a single configuration language. By having a more generic IR Logstash could easily be extended with multiple config languages. The downside of this is that it could fragment the Logstash community and potentially result in non-interchangeable configurations. This would need to be discussed at length and have its Pros and Cons weighed.
  4. Improve Pipeline Introspection: Graphs are a richer construct than ASTs in many ways, and flow analysis can be easily done with textbook graph traversal algorithms. We can leverage the DAG to provide new types of introspection.
  5. Improve Code Modularity: The current design complects execution and representation of pipelines to a large extent. By separating those concerns experimentation becomes easier.
@@ -2,61 +2,7 @@
require 'logstash/errors'
require "treetop"
class Treetop::Runtime::SyntaxNode

This comment has been minimized.

@andrewvc

andrewvc Feb 14, 2017

Contributor

This was all moved to treetop_monkeypatches.

@jordansissel

This comment has been minimized.

Contributor

jordansissel commented Feb 14, 2017

First things first. Is LIR prounounced in a fun way? "Lyre" ? "Ell Eye Arr" ? "Lure" ?

@ph

This comment has been minimized.

Member

ph commented Feb 14, 2017

@jordansissel Lire ? which is the french word for reading.

@andrewvc andrewvc changed the title from LIR to LIR Read Only Feb 14, 2017

@suyograo suyograo added the v6.0.0 label Feb 14, 2017

@andrewvc

This comment has been minimized.

Contributor

andrewvc commented Feb 14, 2017

@jordansissel I say "El 👁 Are" :)

@jordansissel

This comment has been minimized.

Contributor

jordansissel commented Feb 21, 2017

Did a first pass at reviewing the Graph+Edge+Vertex details and provided feedback to @adnrewvc over Zoom.

Next, I will study the test suite.

@jordansissel

This comment has been minimized.

Contributor

jordansissel commented Mar 1, 2017

I cannot run the tests:

% USE_RUBY=1 bin/bundle exec rspec logstash-core/spec/
...
NameError: uninitialized constant SUPPORT_DIR
    const_missing at org/jruby/RubyModule.java:2719
           (root) at /home/jls/projects/logstash/logstash-core/spec/logstash/compiler/compiler_spec.rb:37
      module_exec at org/jruby/RubyModule.java:2356
         subclass at /home/jls/projects/logstash/vendor/bundle/jruby/1.9/gems/rspec-core-3.1.7/lib/rspec/core/example_group.rb:325
         describe at /home/jls/projects/logstash/vendor/bundle/jruby/1.9/gems/rspec-core-3.1.7/lib/rspec/core/example_group.rb:219
           (root) at /home/jls/projects/logstash/logstash-core/spec/logstash/compiler/compiler_spec.rb:20
      module_exec at org/jruby/RubyModule.java:2356
         subclass at /home/jls/projects/logstash/vendor/bundle/jruby/1.9/gems/rspec-core-3.1.7/lib/rspec/core/example_group.rb:325
         describe at /home/jls/projects/logstash/vendor/bundle/jruby/1.9/gems/rspec-core-3.1.7/lib/rspec/core/example_group.rb:219
           (root) at /home/jls/projects/logstash/logstash-core/spec/logstash/compiler/compiler_spec.rb:16
...

rake test:core runs, but this method (rake) does not allow me to run specific tests.

@jordansissel

I've halfway reviewed the test suite and made some more comments.

@@ -0,0 +1,580 @@
require "spec_helper"
require "logstash/compiler"
java_import Java::OrgLogstashConfigIr::DSL

This comment has been minimized.

@jordansissel

jordansissel Mar 2, 2017

Contributor

My preferred syntax here is org.logstash.config.ir.DSL instead of Java::BouncyCaseNameSpaceThing

java_import Java::OrgLogstashConfigIr::DSL
describe LogStash::Compiler do
def j

This comment has been minimized.

@jordansissel

jordansissel Mar 2, 2017

Contributor

Curious. Why call this j? Is this not better written as java_import org.logstash.config.ir.DSL ?

This comment has been minimized.

@andrewvc

andrewvc Mar 22, 2017

Contributor

This is used all over this file, so its here as shorthand to make the tests more readable, while still providing more namespacing safety than import.

if expected.java_kind_of?(org.logstash.config.ir.SourceComponent) && actual.java_kind_of?(org.logstash.config.ir.SourceComponent)
expected.sourceComponentEquals(actual)
else
return false

This comment has been minimized.

@jordansissel

jordansissel Mar 2, 2017

Contributor

is return correct, here? You're returning not from a function, so it's unclear the purpose.

Do you mean to just implicit return false ?

This comment has been minimized.

@andrewvc

andrewvc Mar 22, 2017

Contributor

Good point! Implicit return would be nil, I think for a matcher its better to return false than a falsey value. I will restructure this to be nicer however.

This comment has been minimized.

@andrewvc

andrewvc Mar 22, 2017

Contributor

Oh, and it should be next not return in the first place.

CODEC = Java::OrgLogstashConfigIr::PluginDefinition::Type::OUTPUT
describe "compiling to Pipeline" do
subject(:source_file) { "fake_sourcefile" }

This comment has been minimized.

@jordansissel

jordansissel Mar 2, 2017

Contributor

Can you use let here instead of subject ? It feels weird to have two subjects defined.

shared_examples_for "compilable LSCL files" do |path|
describe "parsing #{path}" do
let(:source) {
File.open(path) {|f| f.read }

This comment has been minimized.

@jordansissel

jordansissel Mar 2, 2017

Contributor

This can be File.read(path)

public class PipelineTest {
@Test
public void testPipelineCreation() throws InvalidIRException {
Graph inputSection = iComposeParallel(iPlugin(INPUT, "generator"), iPlugin(INPUT, "stdin")).toGraph();

This comment has been minimized.

@jordansissel

jordansissel Mar 2, 2017

Contributor

As I read this, the i prefix is causing me to stumble a bit. I'm not quite able to articulate a specific issue right now, though.

This comment has been minimized.

@andrewvc

andrewvc Mar 23, 2017

Contributor

I suppose there is the option to have three separate DSL classes. One each for imperative/graph/expression. That would preclude the ability to use static imports as well though.

end
def empty_source_meta()
Java::OrgLogstashConfigIr::SourceMetadata.new()

This comment has been minimized.

@jordansissel

jordansissel Mar 2, 2017

Contributor

For these java references, I greatly prefer dotted naming org.logstash.config.ir instead of Java::OrgLogstashConfigIr because the former aligns more closely to java namespacing.

class PluginSection < Node
def expr
[*recursive_select(Branch, Plugin).map(&:expr)]

This comment has been minimized.

@jordansissel

jordansissel Mar 2, 2017

Contributor

[*x.map()] is the same as x.map() is it not? Both produce an array value, but the [*...] syntax feels obtuse.

This comment has been minimized.

@andrewvc

andrewvc Mar 22, 2017

Contributor

Yeah, I think this went through a few rewrites and wound up with some redundancy :)

assertVerticesConnected(from, to);
}
public void assertVerticesConnected(Vertex from, Vertex to) {

This comment has been minimized.

@jordansissel

jordansissel Mar 2, 2017

Contributor

should this be in IRHelpers?

This comment has been minimized.

@andrewvc

andrewvc Mar 22, 2017

Contributor

Maybe. They're only used in this file. I'd rather not extract till its needed.

// Will not validate the graph after running!
// You must invoke validate the graph yourself
// after invoking
public Collection<Edge> threadVerticesUnsafe(Edge.EdgeFactory edgeFactory, Vertex... argVertices) throws InvalidIRException {

This comment has been minimized.

@jordansissel

jordansissel Mar 2, 2017

Contributor

"thread" is an odd verb here. Is there etymology or a story to tell about the name?

This comment has been minimized.

@andrewvc

andrewvc Mar 22, 2017

Contributor

I'm renaming these from thread* to chain*. Good point.

@talevy talevy removed their assignment Mar 23, 2017

@jordansissel

This comment has been minimized.

Contributor

jordansissel commented Mar 31, 2017

LGTM given --

  • This is new code, not yet really used, and very likely to change as we see it used in the future
  • The imperative parts will hopefully go away once we move more into the lir/dag execution model where we won't need imperative compilation anymore.

I'm ok moving forward as is.

@suyograo

This comment has been minimized.

Member

suyograo commented Mar 31, 2017

Please only merge this to master. BTW, the tests are failing as well..

LIR UI (Read Only) (#6241)
* Read-only Java IR
* Consistent ID generation as opposed to UUIDs
@andrewvc

This comment has been minimized.

Contributor

andrewvc commented Apr 3, 2017

@suyograo tests were passing under bin/rspec and just needed a small tweak to spec_helper to work with the rake task. Rebased and pushed with those changes.

Jordan gave approval in a comment

@elasticsearch-bot

This comment has been minimized.

elasticsearch-bot commented Apr 3, 2017

Andrew Cholakian merged this into the following branches!

Branch Commits
master 76711a4

@suyograo suyograo deleted the lir branch Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment