NIFI-4504, NIFI-4505 added methods to MapCache API …

[mosermw]

… including keySet, removeAndGet, removeByPatternAndGet
cleaned up some warnings on deprecated nifi.stream.io classes

I attempted to update the various implementations of DistributedMapCacheClient,
but if the reviewer feels I should leave them unsupported then let me know.

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

• Is there a JIRA ticket associated with this PR? Is it referenced
  in the commit message?

• Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

• Has your PR been rebased against the latest commit within the target branch (typically master)?

• Is your initial contribution a single, squashed commit?

For code changes:

• Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
• Have you written or updated unit tests to verify your changes?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
• If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
• If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

• Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

[joewitt]

@mosermw can you rebase and force push so we can get travis to do its thing.

We need someone to review. It might be best to make it smaller by leaving these new methods unsupported/unused in the existing processors if I'm understanding that right.

[ijokarumawak]

Thanks @mosermw for the improvements. I agree with @joewitt to minimize the changes required. I think all implementation classes in Unit test code need not to be updated if DistributedMapChacheClient defines those new methods default behavior.

BTW, this change nor the JIRA description do not mention why/where these improvements are needed. Are these new APIs expected to be used from some Processors? While the JIRA description sounds reasonable, if none needs the new APIs, we shouldn't add it, otherwise we just create YAGNI. Also, since these method implementations in this PR are not truly atomic operations (simply because underlying cache engine does not support it), those may be able to be implemented at the caller side.

[ijokarumawak]

For multiple operations like these get and del, Redis provides multi to make those as an atomic execution. Since other methods are implemented similarly, I don't have strong opinion to use multi here, but we can submit another JIRA for future improvement. How do you think?

http://www.rediscookbook.org/get_and_delete.html
https://stackoverflow.com/questions/11148383/how-to-implement-redis-multi-exec-by-using-spring-data-redis

[ijokarumawak]

This method doesn't look realistic. What if the cache storage contains tens of thousand or more keys? We should avoid loading all keys into heap.

[ijokarumawak]

This new method should have a default implementation which throws UnsupportedOperationException.

[ijokarumawak]

This new method should have a default implementation which throws UnsupportedOperationException.

[mosermw]

Thank you Joe and Koji for taking a look at this. I struggled with deciding whether to provide a default UnsupportedOperationException or to provide the new methods in the various DistributedMapCacheClient implementations. I will happily change this PR to go with the UnsupportedOperationException route. Give me a little time to make the changes and force push, but it should be a much smaller PR and easier to review.

I agree that a keySet method could become problematic in a distributed Map environment. However I believe it's important to provide as much of the Java Map interface in the distributed Map as we can, and let the implementer beware of the potential problems. I do know some folks who have extended DistributedMapCacheClient with their own implementations, and I would like the keySet method to exist to support them.

[mosermw]

I rebased and force pushed the discussed changes. It appears that the Travis failure is unrelated?

[ijokarumawak]

@mosermw Thank you for updating the PR, it's much easier to review now. The code implemented at DistributedMapCacheClientService and MapCacheServer looks good to me. Travis error should be fine as the failure looks depending on execution condition.

However, let me ask one more time since adding methods is easy but hard to remove afterwards for things like this. Are all of these three methods, removeAndGet, removeByPatternAndGet and keySet required by the folks you know of? I prefer to minimize the addition as those methods are only supported by DistributedMapCacheClientService, which means those will not be used by most developers who write Processors. Also, it confuses such developers which to choose from remove and removeAndGet. Probably I imagine these concerns are what made you struggle with implementing default method to just throw UnsupportedOperationException.

If there is no significant needs for removeAndGet and removeByPatternAndGet, then I'd prefer omit these method from DistributedMapCacheClient interface. As long as we support keySet, pretty much everything can be done at the caller side. The only benefit to have AndGet methods I am aware of is reducing the network traffic, and atomicity (atomicity is not that important in these method IMHO). Do you think those are more important than adding mostly unsupported methods into a common interface?

[devriesb]

Koji, "Are all of these three methods, removeAndGet, removeByPatternAndGet and keySet required by the folks you know of?" - Yes. "atomicity is not that important in these method IMHO" - I think you'll find not everyone shares that opinion. Ultimately, removeAndGet behaves the same as the remove() method of the java Map interface, which is the behavior I (and I would suspect many others) would expect. While I understand providing a "flavor" of this method that does not return the removed value to save network traffic, I would have preferred those be named differently. However, since we're now locked into "remove" not returning the removed value, "removeAndGet" is likely as good as we're going to get. Brandon

…

On Wed, Dec 27, 2017 at 7:59 PM Koji Kawamura ***@***.***> wrote: @mosermw <https://github.com/mosermw> Thank you for updating the PR, it's much easier to review now. The code implemented at DistributedMapCacheClientService and MapCacheServer looks good to me. Travis error should be fine as the failure looks depending on execution condition. However, let me ask one more time since adding methods is easy but hard to remove afterwards for things like this. Are all of these three methods, removeAndGet, removeByPatternAndGet and keySet required by the folks you know of? I prefer to minimize the addition as those methods are only supported by DistributedMapCacheClientService, which means those will not be used by most developers who write Processors. Also, it confuses such developers which to choose from remove and removeAndGet. Probably I imagine these concerns are what made you struggle with implementing default method to just throw UnsupportedOperationException. If there is no significant needs for removeAndGet and removeByPatternAndGet, then I'd prefer omit these method from DistributedMapCacheClient interface. As long as we support keySet, pretty much everything can be done at the caller side. The only benefit to have AndGet methods I am aware of is reducing the network traffic, and atomicity (atomicity is not that important in these method IMHO). Do you think those are more important than adding mostly unsupported methods into a common interface? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2284 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB2qyBEO72vOhV9p-2HcFIUoAS3_UM9xks5tEufigaJpZM4Qmhur> .

[ijokarumawak]

@mosermw @devriesb Thanks for sharing your opinions. I think we have enough discussion and since there are multiple audience willing to have these method. I am going to merge this. Thanks again for your contribution and improving the project!