{"action":"created","issue":{"url":"https://api.github.com/repos/elastic/logstash/issues/4123","labels_url":"https://api.github.com/repos/elastic/logstash/issues/4123/labels{/name}","comments_url":"https://api.github.com/repos/elastic/logstash/issues/4123/comments","events_url":"https://api.github.com/repos/elastic/logstash/issues/4123/events","html_url":"https://github.com/elastic/logstash/pull/4123","id":114358491,"number":4123,"title":"[WIP] logstash-core, logstash-core-event, logstash-core-event-java","user":{"login":"colinsurprenant","id":2010,"avatar_url":"https://avatars.githubusercontent.com/u/2010?v=3","gravatar_id":"","url":"https://api.github.com/users/colinsurprenant","html_url":"https://github.com/colinsurprenant","followers_url":"https://api.github.com/users/colinsurprenant/followers","following_url":"https://api.github.com/users/colinsurprenant/following{/other_user}","gists_url":"https://api.github.com/users/colinsurprenant/gists{/gist_id}","starred_url":"https://api.github.com/users/colinsurprenant/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/colinsurprenant/subscriptions","organizations_url":"https://api.github.com/users/colinsurprenant/orgs","repos_url":"https://api.github.com/users/colinsurprenant/repos","events_url":"https://api.github.com/users/colinsurprenant/events{/privacy}","received_events_url":"https://api.github.com/users/colinsurprenant/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"jsvd","id":31809,"avatar_url":"https://avatars.githubusercontent.com/u/31809?v=3","gravatar_id":"","url":"https://api.github.com/users/jsvd","html_url":"https://github.com/jsvd","followers_url":"https://api.github.com/users/jsvd/followers","following_url":"https://api.github.com/users/jsvd/following{/other_user}","gists_url":"https://api.github.com/users/jsvd/gists{/gist_id}","starred_url":"https://api.github.com/users/jsvd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jsvd/subscriptions","organizations_url":"https://api.github.com/users/jsvd/orgs","repos_url":"https://api.github.com/users/jsvd/repos","events_url":"https://api.github.com/users/jsvd/events{/privacy}","received_events_url":"https://api.github.com/users/jsvd/received_events","type":"User","site_admin":false},"milestone":null,"comments":5,"created_at":"2015-10-30T21:22:51Z","updated_at":"2015-11-03T17:55:30Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/elastic/logstash/pulls/4123","html_url":"https://github.com/elastic/logstash/pull/4123","diff_url":"https://github.com/elastic/logstash/pull/4123.diff","patch_url":"https://github.com/elastic/logstash/pull/4123.patch"},"body":"this replaces #4100 and adds the **logstash-core-event-java** gem as pure Java implementation replacement of logstash-core-event\r\n\r\n#### Purpose\r\n\r\nThis is a WIP PR which extracts and separate **logstash-core** and **logstash-core-event** as proper gems and also adds the **logstash-core-event-java** gem which is a drop-in replacement of **logstash-core-even**t. \r\n\r\n#### Expectations\r\n\r\nThis is a WIP PR to gather feedback on the idea. Not everything has been tested and some parts have been duck taped to work with the new structure.\r\n\r\n####  Changes\r\n\r\n- all current **logstash-core gem** code  in now moved in the `logstash-core/` directory. \r\n- all current `Event` related code has been extracted from logstash-core and moved into its own **logstash-core-event** gem into the `logstash-core-event/` directory.\r\n- the Java rewrite of **logstash-core-event** is in **logstash-core-event-java** and can drop-in replace **logstash-core-event** see `logstash-core-event-java/REAMDE.md`.\r\n\r\n**ALSO**\r\n- all specs have been reorganized into the **standard** spec directory structure & naming for all `spec/`, `logstash-core/spec` and `logstash-core-event/spec` directories.\r\n- all spec files are in the exact same path as the corresponding source file. No more discrepancy between the `lib/`  and `spec/` dir structures.\r\n\r\n#### TODO\r\n- [ ] make all tests pass for `bin/rspec logstash-core/spec` and `rake test:install-core`\r\n- [ ] figure correct jar dependencies installation for dev vs release in `logstash-core-event-java/lib/logstash-core-event-java`\r\n- [ ] figure correct jar dependencies loading for dev vs release in `logstash-core-event-java/lib/logstash-core-event-java`\r\n- [ ] fix rake tasks for tests and builds"},"comment":{"url":"https://api.github.com/repos/elastic/logstash/issues/comments/153433055","html_url":"https://github.com/elastic/logstash/pull/4123#issuecomment-153433055","issue_url":"https://api.github.com/repos/elastic/logstash/issues/4123","id":153433055,"user":{"login":"colinsurprenant","id":2010,"avatar_url":"https://avatars.githubusercontent.com/u/2010?v=3","gravatar_id":"","url":"https://api.github.com/users/colinsurprenant","html_url":"https://github.com/colinsurprenant","followers_url":"https://api.github.com/users/colinsurprenant/followers","following_url":"https://api.github.com/users/colinsurprenant/following{/other_user}","gists_url":"https://api.github.com/users/colinsurprenant/gists{/gist_id}","starred_url":"https://api.github.com/users/colinsurprenant/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/colinsurprenant/subscriptions","organizations_url":"https://api.github.com/users/colinsurprenant/orgs","repos_url":"https://api.github.com/users/colinsurprenant/repos","events_url":"https://api.github.com/users/colinsurprenant/events{/privacy}","received_events_url":"https://api.github.com/users/colinsurprenant/received_events","type":"User","site_admin":false},"created_at":"2015-11-03T17:55:30Z","updated_at":"2015-11-03T17:55:30Z","body":"@jsvd thanks for the comments, a few points:\r\n\r\n> To make this review and the changes (which are necessarily huge) manageable, I'd suggest breaking this into 2 PRs: the refactor PR and introduction of the java event PR (which depends on the former)\r\n\r\nI don't think this is necessary since nothing really changed on the core codebase other than relocating files and the *new* code is related to the introduction of the new java event code. Also, it is a pain for me to work on separate branches for changes that are dependant to support & integrate the new java code. Unless this is really impossible to review like this I'd prefer to keep one PR.\r\n\r\n> Have you considered using \"logstash-event\" and \"logstash-event-java\" instead of \"logstash-core-event*\"?\r\n\r\nYes we did, we had a discussion about it with @jordansissel and @suyograo and the problem is that logstash-event already exists as a gem today and to avoid having to deal with this today, we settled on logstash-core-* \r\n\r\n> I just noticed that there are some commited jars that could be downloaded \r\n\r\nYes, this is the 2nd and 3rd point of the TODOs in the PR description.\r\n\r\n> I'm finding this structure awkward to navigate and work with. \r\n\r\nReally? I think it is best like that, let me explain. `logstash-core`, `logstash-core-event` and `logstash-core-event-java` are all self-contained gems, they could in fact live in their own repo but having them in the logstash repo is more convenient. Since they are separate gems, with their own complete dir structure  (`lib/`, `spec/`, ...) I believe the best place is in the root of logstash. the other logstash root dirs like `lib/` and `spec/` are related to the logstash code which is now basically the bootstrapping code, plugin manager and rake libs. \r\n\r\nI am not sure what make it hard to navigate? it might just be a habit change to go through? for me I find it really easy to reason about the \"event\" code which is in `logstash-core-event` and the \"core\" code in `logstash-core` with all related code and specs together where they should be. \r\n\r\nI think its more a about \"accepting\" that the logstash main repo is the shell/rake/bootstrapping/pluginmanager and and the actual code is now modularized into gems and in their own respective directories. \r\n\r\nI know it's always annoying having to adjust to a new dir structure but I believe it does make sense and it is a lot better structured this way. Let me know if this makes sense."}}