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

Add ConfigInputPlugin for easier testing. #678

Merged
merged 3 commits into from Jun 21, 2017

Conversation

Projects
None yet
4 participants
@dmikurube
Contributor

dmikurube commented Jun 15, 2017

When quick-testing in developing the core or plugins (later than input), it is often annoying to set up input. Prepare CSV on a filesystem, confirm the path, ...

An input which does not depend on external resources may help solving the situation. This is it. An input from the Embulk config itself. :)

in:
  type:
    name: config
  columns:
  - {name: id, type: long}
  - {name: name, type: string}
  values:
  - - [ 12, "foo" ]
    - [ 24, "bar" ]
  - - [ 98, "hoge" ]
    - [ 21, "fuga" ]
out:
  type: stdout
2017-06-16 17:34:41.041 +0900 [INFO] (0001:transaction): Using local thread executor with max_threads=8 / output tasks 4 = input tasks 2 * 2
2017-06-16 17:34:41.053 +0900 [INFO] (0001:transaction): {done:  0 / 2, running: 0}
98,hoge
12,foo
21,fuga
24,bar
2017-06-16 17:34:41.130 +0900 [INFO] (0001:transaction): {done:  2 / 2, running: 0}
2017-06-16 17:34:41.130 +0900 [INFO] (0001:transaction): {done:  2 / 2, running: 0}
2017-06-16 17:34:41.138 +0900 [INFO] (main): Committed.
2017-06-16 17:34:41.138 +0900 [INFO] (main): Next config diff: {"in":{},"out":{}}

@dmikurube dmikurube changed the title from [WIP] Add ConfigInputPlugin for easier testing. to Add ConfigInputPlugin for easier testing. Jun 16, 2017

@dmikurube dmikurube requested a review from muga Jun 16, 2017

@dmikurube

This comment has been minimized.

Show comment
Hide comment
@dmikurube

dmikurube Jun 16, 2017

Contributor

@muga Can I have your thoughts? I often wanted this kind of preset plugins...

Contributor

dmikurube commented Jun 16, 2017

@muga Can I have your thoughts? I often wanted this kind of preset plugins...

@civitaspo

This comment has been minimized.

Show comment
Hide comment
@civitaspo

civitaspo Jun 16, 2017

Member

[Just Share]
This plugin has the similar function > https://github.com/toyama0919/embulk-input-inline

Member

civitaspo commented Jun 16, 2017

[Just Share]
This plugin has the similar function > https://github.com/toyama0919/embulk-input-inline

@dmikurube

This comment has been minimized.

Show comment
Hide comment
@dmikurube

dmikurube Jun 16, 2017

Contributor

Ah, thanks for sharing! I was not sure about that plugin.

Let me share the reason why I wrote this as a preset standard plugin: it's because I often wanted this kind of plugins during core development. Installing external plugins was not very convenient when testing the core itself...

Contributor

dmikurube commented Jun 16, 2017

Ah, thanks for sharing! I was not sure about that plugin.

Let me share the reason why I wrote this as a preset standard plugin: it's because I often wanted this kind of plugins during core development. Installing external plugins was not very convenient when testing the core itself...

@civitaspo

This comment has been minimized.

Show comment
Hide comment
@civitaspo

civitaspo Jun 16, 2017

Member

I see! I thought that core committers might have a policy that does not increase plugins having similar functions like embulk-parser-jsonl, so I shared.
Actually I also thought that it was better for Embulk Core to have the function of this plugin. So this pull request is greatly appreciated.

Member

civitaspo commented Jun 16, 2017

I see! I thought that core committers might have a policy that does not increase plugins having similar functions like embulk-parser-jsonl, so I shared.
Actually I also thought that it was better for Embulk Core to have the function of this plugin. So this pull request is greatly appreciated.

@muga

This comment has been minimized.

Show comment
Hide comment
@muga

muga Jun 16, 2017

Contributor

@dmikurube @civitaspo This feature would be helpful for Embulk development 👍 In particular, it would be great if Embulk core could be separately tested without any plugins installation. Plugins installation and distribution ways might be changed.

Do you think that I can start reviewing this and we will merge? As an existing plugin that has a similar functionality could be found, additional steps are needed before reviewing?

a policy that does not increase plugins having similar functions like embulk-parser-jsonl, so I shared.

Make sense but, I'm still not sure what type of a policy or document we need to have as Embulk public site? It's better to create another ticket separately. cc: @hiroyuki-sato

Contributor

muga commented Jun 16, 2017

@dmikurube @civitaspo This feature would be helpful for Embulk development 👍 In particular, it would be great if Embulk core could be separately tested without any plugins installation. Plugins installation and distribution ways might be changed.

Do you think that I can start reviewing this and we will merge? As an existing plugin that has a similar functionality could be found, additional steps are needed before reviewing?

a policy that does not increase plugins having similar functions like embulk-parser-jsonl, so I shared.

Make sense but, I'm still not sure what type of a policy or document we need to have as Embulk public site? It's better to create another ticket separately. cc: @hiroyuki-sato

@hiroyuki-sato

This comment has been minimized.

Show comment
Hide comment
@hiroyuki-sato

hiroyuki-sato Jun 17, 2017

Contributor

In particular, it would be great if Embulk core could be separately tested without any plugins installation.

I think so too.

It would be great if this plugin developer can also use this extension in the near future.
The Embulk community is a small community. It is better to cooperate with each other.
Many plugin developers are looking for how to test my plugin. This document(How to test your plugin) may help all of the plugin developers.
(I'll create another ticket about this).

Contributor

hiroyuki-sato commented Jun 17, 2017

In particular, it would be great if Embulk core could be separately tested without any plugins installation.

I think so too.

It would be great if this plugin developer can also use this extension in the near future.
The Embulk community is a small community. It is better to cooperate with each other.
Many plugin developers are looking for how to test my plugin. This document(How to test your plugin) may help all of the plugin developers.
(I'll create another ticket about this).

@dmikurube

This comment has been minimized.

Show comment
Hide comment
@dmikurube

dmikurube Jun 17, 2017

Contributor

@muga Yeah, I think it's okay to start reviewing, while we may want to implement another config-style in maps (not arrays) like embulk-input-inline.

Users need to change their configurations anyways, though, because one difference from embulk-input-inline is that this ConfigInputPlugin takes control of task separation.

@hiroyuki-sato Thanks! Yeah, we need documents for this.

Contributor

dmikurube commented Jun 17, 2017

@muga Yeah, I think it's okay to start reviewing, while we may want to implement another config-style in maps (not arrays) like embulk-input-inline.

Users need to change their configurations anyways, though, because one difference from embulk-input-inline is that this ConfigInputPlugin takes control of task separation.

@hiroyuki-sato Thanks! Yeah, we need documents for this.

@muga

This comment has been minimized.

Show comment
Hide comment
@muga

muga Jun 19, 2017

Contributor

@hiroyuki-sato thank you. What do you think about @civitaspo's first thought?

@dmikurube Ok, will take a look.

Contributor

muga commented Jun 19, 2017

@hiroyuki-sato thank you. What do you think about @civitaspo's first thought?

@dmikurube Ok, will take a look.

@muga

@dmikurube Basically looks good to me. I left one comment. Please check it.

@muga

muga approved these changes Jun 20, 2017

LGTM 👍 Thank you for fixing.

@muga

This comment has been minimized.

Show comment
Hide comment
@muga

muga Jun 20, 2017

Contributor

Started rebuilding the failed CI jobs.

Contributor

muga commented Jun 20, 2017

Started rebuilding the failed CI jobs.

@hiroyuki-sato

This comment has been minimized.

Show comment
Hide comment
@hiroyuki-sato

hiroyuki-sato Jun 20, 2017

Contributor

@muga

Actually I also thought that it was better for Embulk Core to have the function of this plugin. So this pull request is greatly appreciated.

Agree!. And I hope plugin developer can use this plugin for their plugin test in the near future.

We shouldn't develop another plugin which has similar functions as much as we can.
If the embulk-input-inline has useful feature, I hope to merge that in this plugin and maintain it with him(Mr. toyama0919)

Contributor

hiroyuki-sato commented Jun 20, 2017

@muga

Actually I also thought that it was better for Embulk Core to have the function of this plugin. So this pull request is greatly appreciated.

Agree!. And I hope plugin developer can use this plugin for their plugin test in the near future.

We shouldn't develop another plugin which has similar functions as much as we can.
If the embulk-input-inline has useful feature, I hope to merge that in this plugin and maintain it with him(Mr. toyama0919)

@dmikurube

This comment has been minimized.

Show comment
Hide comment
@dmikurube

dmikurube Jun 20, 2017

Contributor

@hiroyuki-sato Thanks for adding your comment. (Cc: @muga)

AFAIU, major differences from embulk-input-inline are:

  1. embulk-input-inline is written in Ruby while ConfigInputPlugin is in Java.
  2. embulk-input-inline accepts data in mapping while ConfigInputPlugin accepts in an array.
  3. embulk-input-inline always sends data in one task while ConfigInputPlugin controls tasks.

If you're fine, I'd merge this PR soon.

2 is worth importing from embulk-input-inline. At first I thought to implement 2 in this PR, but it'd be okay to add later.

Contributor

dmikurube commented Jun 20, 2017

@hiroyuki-sato Thanks for adding your comment. (Cc: @muga)

AFAIU, major differences from embulk-input-inline are:

  1. embulk-input-inline is written in Ruby while ConfigInputPlugin is in Java.
  2. embulk-input-inline accepts data in mapping while ConfigInputPlugin accepts in an array.
  3. embulk-input-inline always sends data in one task while ConfigInputPlugin controls tasks.

If you're fine, I'd merge this PR soon.

2 is worth importing from embulk-input-inline. At first I thought to implement 2 in this PR, but it'd be okay to add later.

@dmikurube

This comment has been minimized.

Show comment
Hide comment
@dmikurube

dmikurube Jun 20, 2017

Contributor

(In addition, I actually thought inline was really a better name. I couldn't rename ConfigInputPlugin to inline since we should avoid naming conflicts... ;) )

Contributor

dmikurube commented Jun 20, 2017

(In addition, I actually thought inline was really a better name. I couldn't rename ConfigInputPlugin to inline since we should avoid naming conflicts... ;) )

@muga

This comment has been minimized.

Show comment
Hide comment
@muga

muga Jun 20, 2017

Contributor

Hm. As we should solve to avoid name conflicts later, if you strongly want to use the name, we could negotiate and ask him to transfer it but..

Contributor

muga commented Jun 20, 2017

Hm. As we should solve to avoid name conflicts later, if you strongly want to use the name, we could negotiate and ask him to transfer it but..

@dmikurube

This comment has been minimized.

Show comment
Hide comment
@dmikurube

dmikurube Jun 21, 2017

Contributor

@muga I didn't mean I seriously want to rename... No problems. :)

Contributor

dmikurube commented Jun 21, 2017

@muga I didn't mean I seriously want to rename... No problems. :)

@dmikurube

This comment has been minimized.

Show comment
Hide comment
@dmikurube

dmikurube Jun 21, 2017

Contributor

Thanks for taking a look. Merging this.

Contributor

dmikurube commented Jun 21, 2017

Thanks for taking a look. Merging this.

@dmikurube dmikurube merged commit 9cdc2bb into master Jun 21, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@dmikurube dmikurube deleted the config-input-plugin branch Jun 21, 2017

@dmikurube dmikurube modified the milestone: v0.8.26 Jun 27, 2017

@dmikurube dmikurube added the kaizen label Jun 27, 2017

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