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

IAtomicReference#alter does not persist changes #8149

Closed
KIC opened this issue May 11, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@KIC
Copy link

commented May 11, 2016

For some reason when I just want to alter an AtomicReference like so:

  ref.alter(node -> {
            node.nextNodes[0] = lala;
            node.nextKeys[0] = "lala";
            return node;
        });

then nothing changes. A later get will still return the old object. But when i change this to alterAndGet

  ref.alterAndGet(node -> {
            node.nextNodes[0] = lala;
            node.nextKeys[0] = "lala";
            return node;
        });

then everything works as expected. But in fact I do not want so send the whole object over wire, I just want to modify a property.

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented May 11, 2016

@KIC: thanks for reporting this issue. What Hazelcast version are you using? Is it with client-member or member-member topology?

@KIC

This comment has been minimized.

Copy link
Author

commented May 11, 2016

I am using 3.6.2 but I also could reproduce it with 3.5.5. I normally use a client, server setup but the issue remains by just using one local server.

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented May 11, 2016

@KIC: interesting, we have integration tests for this running with each change. see https://github.com/hazelcast/hazelcast/blob/v3.5.5/hazelcast/src/test/java/com/hazelcast/concurrent/atomicreference/AtomicReferenceBasicTest.java

Can you please provide a reproducer? thanks!

@jerrinot jerrinot added this to the 3.7 milestone May 11, 2016

@KIC

This comment has been minimized.

Copy link
Author

commented May 11, 2016

Sure:

package com.stuff.hazelcast;

import com.hazelcast.core.Hazelcast;
import com.hazelcast.core.HazelcastInstance;
import com.hazelcast.core.IAtomicReference;

import java.io.Serializable;
import java.util.Arrays;

/**
 * Created by kic on 11.05.16.
 */
public class Issue {
    public static class MyClass implements Serializable {
        private static final long serialVersionUID = -3394196400279204634L;
        public String[] someArray;

        public MyClass() {
            this.someArray = new String[10];
        }

        @Override
        public String toString() {
            return Arrays.toString(someArray);
        }
    }

    public static void main(String[] args) {
        HazelcastInstance hi = Hazelcast.newHazelcastInstance();
        IAtomicReference<MyClass> foo = hi.getAtomicReference("foo");

        foo.set(new MyClass());

        foo.alter(mc -> {
            mc.someArray[0] = "hello";
            return mc;
        });

        System.out.println(foo.get()); // prints [null, null, null, null, null, null, null, null, null, null]

        foo.alterAndGet(mc -> {
            mc.someArray[0] = "hello";
            return mc;
        });

        System.out.println(foo.get()); // prints [hello, null, null, null, null, null, null, null, null, null]

    }
}
@jerrinot

This comment has been minimized.

Copy link
Contributor

commented May 11, 2016

@KIC: Many thanks, there is really a bug which manifests itself only when your function mutates the original object instead of returning a new instance. The same issue was already fixed for alterAndGet(), see #2495

I'll send a fix for alter() too. In meantime you can use this as a workaround:

        foo.alter(mc -> {
            MyClass copy = new MyClass();
            copy.someArray = mc.someArray;
            copy.someArray[0] = "hello";
            return copy;
        });

@jerrinot jerrinot self-assigned this May 11, 2016

jerrinot added a commit to jerrinot/hazelcast that referenced this issue May 11, 2016

Multiple IAtomicReference issues fixed
1. getAndAlter test was testing alterAndGet method
2. AlterAndGetOperation optimized - sen backup only when (serialized) new value is different from the original value
3. AlterOperation fixed - use serialized values to make a decision whether to persist change - otherwise a change is lost when the function mutates the original object instead of creating a backup
4. GetAndAlterOperation fixed - the same issues as with AlterOperation

Fixes hazelcast#8149

jerrinot added a commit to jerrinot/hazelcast that referenced this issue May 11, 2016

Multiple IAtomicReference issues fixed
1. getAndAlter test was testing alterAndGet method
2. AlterAndGetOperation optimized - sen backup only when (serialized) new value is different from the original value
3. AlterOperation fixed - use serialized values to make a decision whether to persist change - otherwise a change is lost when the function mutates the original object instead of creating a backup
4. GetAndAlterOperation fixed - the same issues as with AlterOperation

Fixes hazelcast#8149
(cherry picked from commit 2ad2e7b)
@KIC

This comment has been minimized.

Copy link
Author

commented May 12, 2016

Hmmm ... I have now just tried to add an equals() { return false; } to my object but it seems to be the only workaround to make a new/clone object. However does this mean with apply I am not writing to the original object and every update leads to a serialization (on the same node, without any transfer over wire) of a potentially big object graph? Could I configure that behavior?

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

@KIC: your are right. There is always a serialization. However bear in mind the cost of (de-)serialization is usually dwarfed by a network latency. Also when your function actually modifies an object then we need to serialize it anyway - as we need to send a backup over a network.

I agree there is still a room for optimization - for the cases where the function does not do any modification hence there is no need to send a backup. The only question is whether it's worth it.

It boils down to the problem of detecting a modification in an object graph. I see these options:

  1. Hazelcast could do what Hibernate does: Instrument an object and try to detect changes
  2. Provide a new interface a user could implement. Something along these lines:
public interface ModCountable {
  /**
  * Return monotonically increasing sequence counting object modifications.
 */
  int getModCount();
}

The first approach feels to be rather complex & fragile. It's also hard to implement it without introducing external dependencies (CGLib, asm, etc..)

The 2nd approach is trivial to implement, but I'm always reluctant to introduce a new public interface - as each new interface means additional complexity for our end-user. Simplicity is our key value.

There is yet another option: We could change a contract so the function would not be allowed to mutated a passed object and it would be required to return a new instance (on modification) - then it's again trivial to detect changes. However this is a compatibility breaking change -> it's not possible.

I'd recommend you to use an optimized serialization method for you domain object - Java Serialization has not been really designed for space/time effectivity. Have a look at DataSerializable or IdentifiedDataSerializable - they are usually much faster - that should do the trick.

Feel free to re-open this ticket if you unnecessary serialization become your bottleneck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.