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

Ispn 7900 Provide entrySet, values, keySet implementation for RemoteCache #5211

Merged
merged 3 commits into from Jun 23, 2017

Conversation

Projects
None yet
6 participants
@wburns
Contributor

wburns commented Jun 16, 2017

@wburns wburns requested review from galderz and tristantarrant Jun 16, 2017

@wburns wburns added this to the 9.1.0.Final milestone Jun 16, 2017

@rvansa

While the getVersioned in removeEntry is suboptimal, I think we could live with that. Removing through values().forEach() would be a total disaster, though; I think you need to pass the whole entry to the removal callback.

@rvansa

rvansa approved these changes Jun 19, 2017

I think the filters for keys would be still beneficial, but first version can get in without that.

@wburns

This comment has been minimized.

Show comment
Hide comment
@wburns

wburns Jun 19, 2017

Contributor

Updated per comments, still looking into filter/converter though as this was only thing not done.

Contributor

wburns commented Jun 19, 2017

Updated per comments, still looking into filter/converter though as this was only thing not done.

@wburns

This comment has been minimized.

Show comment
Hide comment
@wburns

wburns Jun 19, 2017

Contributor

Added in key value filter converter as well for keySet.

Contributor

wburns commented Jun 19, 2017

Added in key value filter converter as well for keySet.

@wburns

This comment has been minimized.

Show comment
Hide comment
@wburns

wburns Jun 20, 2017

Contributor

Fixing some test failures as well, will have to wait for run to verify they pass after I update PR.

Contributor

wburns commented Jun 20, 2017

Fixing some test failures as well, will have to wait for run to verify they pass after I update PR.

@wburns

This comment has been minimized.

Show comment
Hide comment
@wburns

wburns Jun 20, 2017

Contributor

Okay fixed test failures and recent comments. This has to wait for a CI run to verify no other failures though.

Contributor

wburns commented Jun 20, 2017

Okay fixed test failures and recent comments. This has to wait for a CI run to verify no other failures though.

@rvansa

rvansa approved these changes Jun 21, 2017

Mostly nitpicking.

Show outdated Hide outdated ...ent/src/main/java/org/infinispan/client/hotrod/impl/RemoteCacheImpl.java Outdated
Show outdated Hide outdated ...ent/src/main/java/org/infinispan/client/hotrod/impl/RemoteCacheImpl.java Outdated
@Override
public boolean remove(Object o) {
Map.Entry<K, V> entry = toEntry(o);
return entry != null && RemoteCacheImpl.this.removeEntry(entry);

This comment has been minimized.

@rvansa

rvansa Jun 21, 2017

Member

RemoteCacheSupport contains remove(K key, V value) which throws UnsupportedOperationException. I think either this method should throw that, too (and then removeEntry could be deleted) or plain RemoteCache.remove(K, V) should be supported, too.

@rvansa

rvansa Jun 21, 2017

Member

RemoteCacheSupport contains remove(K key, V value) which throws UnsupportedOperationException. I think either this method should throw that, too (and then removeEntry could be deleted) or plain RemoteCache.remove(K, V) should be supported, too.

This comment has been minimized.

@wburns

wburns Jun 21, 2017

Contributor

I had originally implemented remove(K key, V value) to do what my added removeEntry does. The problem is if I added that method I feel like I also would have to add all the various replace methods which I wasn't looking forward to.

WDYT?

@wburns

wburns Jun 21, 2017

Contributor

I had originally implemented remove(K key, V value) to do what my added removeEntry does. The problem is if I added that method I feel like I also would have to add all the various replace methods which I wasn't looking forward to.

WDYT?

This comment has been minimized.

@rvansa

rvansa Jun 21, 2017

Member

In that case just don't support removal on the collection. Though, showing the 'correct' replace wouldn't hurt too much, especially if you include a javadoc comment that users should prefer getWithMetadata + replaceWithVersion.

@rvansa

rvansa Jun 21, 2017

Member

In that case just don't support removal on the collection. Though, showing the 'correct' replace wouldn't hurt too much, especially if you include a javadoc comment that users should prefer getWithMetadata + replaceWithVersion.

This comment has been minimized.

@wburns

wburns Jun 21, 2017

Contributor

I will add replace, it shouldn't be too bad.

@wburns

wburns Jun 21, 2017

Contributor

I will add replace, it shouldn't be too bad.

ISPN-7900 Provide entrySet, values, keySet implementation for
RemoteCache

* Refactor removable iterator to be more flexible
* Allows for remove to be more specific including value if needed
@wburns

This comment has been minimized.

Show comment
Hide comment
@wburns

wburns Jun 21, 2017

Contributor

Addressed comments, also added in support for replace and remove overridden methods on RemoteCache.

Contributor

wburns commented Jun 21, 2017

Addressed comments, also added in support for replace and remove overridden methods on RemoteCache.

* This method requires 2 round trips to the server. The first to retrieve the value and version and a second to
* replace the key with the version if the value matches. If possible user should use
* {@link RemoteCache#getWithMetadata(Object)} and
* {@link RemoteCache#replaceWithVersion(Object, Object, long, long, TimeUnit, long, TimeUnit)} if possible.

This comment has been minimized.

@tristantarrant

tristantarrant Jun 21, 2017

Member

cacophony alert: if possilbe .... if possible

@tristantarrant

tristantarrant Jun 21, 2017

Member

cacophony alert: if possilbe .... if possible

* This method requires 2 round trips to the server. The first to retrieve the value and version and a second to
* replace the key with the version if the value matches. If possible user should use
* {@link RemoteCache#getWithMetadata(Object)} and
* {@link RemoteCache#replaceWithVersion(Object, Object, long, long, TimeUnit, long, TimeUnit)} if possible.

This comment has been minimized.

@tristantarrant

tristantarrant Jun 21, 2017

Member

cacophony alert: if possilbe .... if possible

@tristantarrant

tristantarrant Jun 21, 2017

Member

cacophony alert: if possilbe .... if possible

wburns added some commits Jun 13, 2017

ISPN-7900 Provide entrySet, values, keySet implementation for
RemoteCache

* Implement actual backing collections
* Add tests for each method and various modes
ISPN-7900 Provide entrySet, values, keySet implementation for
RemoteCache

* Added user guide updates
* Added section to upgrade guide
@tristantarrant

This comment has been minimized.

Show comment
Hide comment
@tristantarrant

tristantarrant Jun 21, 2017

Member

This needs documentation bits:

  • update the "basic api" paragraph related to bulk ops in the Java Hot Rod client chapter
  • update the upgrading guide to indicate that these bulk ops no longer fetch everything
Member

tristantarrant commented Jun 21, 2017

This needs documentation bits:

  • update the "basic api" paragraph related to bulk ops in the Java Hot Rod client chapter
  • update the upgrading guide to indicate that these bulk ops no longer fetch everything
@tristantarrant

This comment has been minimized.

Show comment
Hide comment
@tristantarrant

tristantarrant Jun 21, 2017

Member

Age is a terrible thing, I completely missed that. LGTM

Member

tristantarrant commented Jun 21, 2017

Age is a terrible thing, I completely missed that. LGTM

@alanfx

This comment has been minimized.

Show comment
Hide comment
@alanfx

alanfx Jun 21, 2017

Contributor

Age beats the alternative!

Contributor

alanfx commented Jun 21, 2017

Age beats the alternative!

@gustavonalle

This comment has been minimized.

Show comment
Hide comment
@gustavonalle

gustavonalle Jun 22, 2017

Contributor

@wburns WDYT of an overload keySet/EntrySet/values that takes a KVfilterConverter string?

Contributor

gustavonalle commented Jun 22, 2017

@wburns WDYT of an overload keySet/EntrySet/values that takes a KVfilterConverter string?

@tristantarrant tristantarrant modified the milestones: 9.1.0.CR1, 9.1.0.Final Jun 22, 2017

@wburns

This comment has been minimized.

Show comment
Hide comment
@wburns

wburns Jun 22, 2017

Contributor

@gustavonalle I think it could be interesting, however it might cause more confusion. This would only affect iterator, spliterator and stream, other methods would be unaffected. Also we have to return a Collection<Object> etc. since we can't rely on the generics any longer.

If we provided this, we should also provide a way to override the batchSize as well I would think.

But either way I am not so sure about that addition in this PR. Maybe we can think about this further on dev list and create another issue?

Contributor

wburns commented Jun 22, 2017

@gustavonalle I think it could be interesting, however it might cause more confusion. This would only affect iterator, spliterator and stream, other methods would be unaffected. Also we have to return a Collection<Object> etc. since we can't rely on the generics any longer.

If we provided this, we should also provide a way to override the batchSize as well I would think.

But either way I am not so sure about that addition in this PR. Maybe we can think about this further on dev list and create another issue?

@gustavonalle

This comment has been minimized.

Show comment
Hide comment
@gustavonalle

gustavonalle Jun 22, 2017

Contributor

@wburns yes, I wasn't thinking about adding it to this PR

Contributor

gustavonalle commented Jun 22, 2017

@wburns yes, I wasn't thinking about adding it to this PR

@tristantarrant tristantarrant merged commit ff878d0 into infinispan:master Jun 23, 2017

1 check was pending

continuous-integration/jenkins/pr-merge This commit is being built
Details
@tristantarrant

This comment has been minimized.

Show comment
Hide comment
@tristantarrant

tristantarrant Jun 23, 2017

Member

Merged to master. Thanks @wburns

Member

tristantarrant commented Jun 23, 2017

Merged to master. Thanks @wburns

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