Skip to content
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-6773: treat non-MetadataAware InternalCacheEntries as not needin… #4412

Closed
wants to merge 5 commits into from

Conversation

ksobolew
Copy link

@ksobolew ksobolew commented Jun 13, 2016

…g the entire Metadata object to be stored.

https://issues.jboss.org/browse/ISPN-6773

  • Make CacheEntry not implement MetadataAware
    • Now only the Metadata_CacheEntry classes (like MetadataMortalCacheEntry)
      implement this interface, while non-Metadata_ classes (like the plain
      MortalCacheEntry) do not.
    • The getMetadata() and setMetadata() method need to be specified in
      CacheEntry interface, as they used to be inherited from MetadataAware.
  • Make InternalEntryFactoryImpl.isStoreMetadata() check the type of the
    InternalCacheEntry in addition to the Metadata object and return false if it
    does not implement MetadataAware.
  • If the entry is new, pass null as the InternalCacheEntry and test only the
    Metadata object.

This way the correct code path (fot Metadata* or non-Metadata*) in
EntryWrappingInterceptor is used even if the Functional API replaces the
Metadata object inside the InternalCacheEntry.

@galderz
Copy link
Member

galderz commented Jun 17, 2016

I'm sorry but we can't accept the change as is. CacheEntry is a public API that we expose and your change could cause issues to existing applications. I agree the interface does not make as much sense as it should (see ISPN-3460) but we can just change it like that. So, I'd suggest you find an alternative way to do what you want to do. I can certainly help you find an alternative way, but you should post a test that demonstrates why you need this change.

@ksobolew
Copy link
Author

@galderz see 75d7d85 for the test case. In my case the EmbeddedMetadata object is created by the CacheLoader, not using the put-with-metadata API, but the result is the same.

@galderz
Copy link
Member

galderz commented Jun 20, 2016

@ksobolew I've sent a PR to your Infinispan repo to workaround this. Have a look and see what you think. The code in InternalEntryFactoryImpl is already making similar assumptions, so I think it should be fine.

@ksobolew
Copy link
Author

@galderz Well, it looks like a workaround all right :) But if it's OK for you then it's OK for me

@galderz
Copy link
Member

galderz commented Jul 1, 2016

@ksobolew Seems like the PR needs rebasing, can you look into that?

@slaskawi slaskawi self-assigned this Jul 1, 2016
Krzysztof Sobolewski and others added 5 commits July 4, 2016 10:25
…g the entire Metadata object to be stored.

* Make CacheEntry *not* implement MetadataAware
  - Now only the Metadata*CacheEntry classes (like MetadataMortalCacheEntry)
    implement this interface, while non-Metadata* classes (like the plain
    MortalCacheEntry) do not.
  - The getMetadata() and setMetadata() method need to be specified in
    CacheEntry interface, as they used to be inherited from MetadataAware.
* Make InternalEntryFactoryImpl.isStoreMetadata() check the type of the
  InternalCacheEntry in addition to the Metadata object and return false if it
  does not implement MetadataAware.
* If the entry is new, pass null as the InternalCacheEntry and test only the
  Metadata object.

This way the correct code path (fot Metadata* or non-Metadata*) in
EntryWrappingInterceptor is used even if the Functional API replaces the
Metadata object inside the InternalCacheEntry.
…ng overwritten by Functional API.

This is the actual scenario where the bug fixed by ISPN-6773 manifests itself.
@ksobolew
Copy link
Author

ksobolew commented Jul 4, 2016

@galderz @slaskawi Rebased.

@slaskawi
Copy link
Contributor

slaskawi commented Jul 6, 2016

Looking

@slaskawi
Copy link
Contributor

slaskawi commented Jul 6, 2016

Integrated thanks!

@slaskawi slaskawi closed this Jul 6, 2016
@@ -3,7 +3,7 @@
import org.infinispan.metadata.Metadata;

/**
* Metdata aware cache entry.
* Marker interface for metadata aware cache entry.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's no longer a marker interface :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danberindei well, yeah, I planned on doing a cleanup (there's also an orphaned import somewhere there) while rebasing but forgot about it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants