Skip to content
New issue

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

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

Already on GitHub? # to your account

setCacheProviderOrIgnore in transaction #58

Merged

Conversation

vietnguyen-td
Copy link
Contributor

Hi,
we have a service that one plugin instance can run multiple times in the same process. That makes the error Cache provider must be configured before cache is accessed, because on the second time the cache was already not null https://code.yawk.at/com.jayway.jsonpath/json-path/2.4.0/com/jayway/jsonpath/spi/cache/CacheProvider.java#14

This change to make safely ignored when cache has already been created before

@vietnguyen-td
Copy link
Contributor Author

@dmikurube could you review this first?

@dmikurube
Copy link
Member

dmikurube commented Mar 1, 2022

@civitaspo Sorry for sending the sudden PR -- it's from us, Treasure Data. Its motivation may be unclear for you. Let me follow up.

As @vietnguyen-td wrote in the PR description, we internally have an Embulk-based microservice that runs Embulk and plugins multiple times in the same process. Such a way is actually discouraged about Embulk, so we're trying to stop such a use. (You may imagine that it, however, still needs time to change such a thing, and we still have to struggle with that yet... 😢)

This proposed change helps the situation of ours because JsonPath's CacheProvider keeps cached items in a static context. The second and subsequent runs of this plugin now fails because it tries to setCacheProvider every time.

On the other hand, this proposed change should have no impacts about ordinary one-shot usage of Embulk.

What do you think?

@civitaspo
Copy link
Member

@vietnguyen-td @dmikurube Sorry for the late reply, I wasn't aware of the PR coming in, I understand the issue you're having with Treasure Data. I don't think this change will affect the plugin users in any way, so I'll merge it.

@civitaspo civitaspo merged commit 5769d1f into embulk:main Mar 1, 2022
@civitaspo civitaspo mentioned this pull request Mar 1, 2022
@civitaspo
Copy link
Member

@vietnguyen-td @dmikurube released https://github.com/civitaspo/embulk-filter-expand_json/releases/tag/v0.5.4

@dmikurube
Copy link
Member

Thanks!

1 similar comment
@vietnguyen-td
Copy link
Contributor Author

Thanks!

@vietnguyen-td vietnguyen-td deleted the set-cache-or-ignore-in-transaction branch March 2, 2022 10:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants