aerospike / aerospike-client-java

Aerospike Java Client Library
Other
236 stars 212 forks source link

Making IAerospikeClient extend java.io.Serializable #91

Closed fpopic closed 6 years ago

fpopic commented 6 years ago

Made IAerospikeClient extend java.io.Serializable with extending also all dependencies (only supertypes) that have been used by the interface.

The goal was to enable serialization of the IAerospikeClient in order to send it over the network to multiple worker machines while using Apache Beam.

Serialization was tested in a real environment using Google Dataflow / Apache Beam machines but also with an easy but good enough example of saving the client to a file and reading it back using serialization.

    val client: IAerospikeClient = { ... }
    val fos = new FileOutputStream("client")
    val oos = new ObjectOutputStream(fos)

    oos.writeObject(client)

    val fis = new FileInputStream("client")
    val ois = new ObjectInputStream(fis)

    val deserializedClient = ois.readObject().asInstanceOf[IAerospikeClient]

To solve https://github.com/aerospike/aerospike-client-java/issues/90

fpopic commented 6 years ago

Mocking the serialized client is working with Mockito "org.mockito" % "mockito-core" % "1.10.19"

with the flag withSettings().serializable()

val client: IAerospikeClient = mock(classOf[IAerospikeClient], withSettings().serializable())

MockSettings serializable() Configures the mock to be serializable. With this feature you can use a mock in a place that requires dependencies to be serializable. WARNING: This should be rarely used in unit testing.

The behaviour was implemented for a specific use case of a BDD spec that had an unreliable external dependency. This was in a web environment and the objects from the external dependency were being serialized to pass between layers.

So now IAerospikeClient interface is 100% serializable, but mock[IAerospikeClient] object or concrete implementation AerospikeClient is not, if you don't add that flag.

Probably these testing libraries (with Scalamock is even worse) have a problem with making a mock that has to be serializable but that's for another topic, the goal of this pull request was 100% achieved!

BrianNichols commented 6 years ago

Your pull request has been accepted. It will appear in the next Java client release.

fpopic commented 6 years ago

Closes #90 (I guess it will automatically close the issue, sorry for closing and reopening)

Aloren commented 6 years ago

Are you sure that this is a good idea to serialize client over network? :)

BrianNichols commented 6 years ago

Good question. What are your arguments against it?

Aloren commented 6 years ago

So my thoughts are:

  1. Why guys do really need to serialize client? Maybe they need to serialize only settings so that they could initialize connection on other machines?
  2. Adding new classes that are going to be used in AerospikeClient will also require them to be marked as Serializable.
  3. Java serialization is quite fragile in terms of backward compatibility. Type Changes Affecting Serialization
  4. After marking these classes Serializiable, some users may actually use this as a feature. In case in the future you would like to remove that -- will it be easy?
  5. IMHO serialization is a means for storing/sending dtos. AerospikeClient is not that type of class.
Aloren commented 6 years ago

Moreover serialized instance of AerospikeClient is not usable on other node, cause it contains tcp connection. This connection will need to be reinitialized on other node to be operable. :confused:

BrianNichols commented 6 years ago

@fpopic Can you just serialize your configuration/hosts, so AerospikeClient could be recreated on the remote machine?

What is your response to these issues?

fpopic commented 6 years ago

Hey, the purpose of this pull request was NOT to make AerospikeClient class serializable, it was only to make the IAerospikeClient interface serializable, to enable the possibility of creating dummy / mocked implementation in testing purposes as I mentioned before. There wouldn't be any concrete objects (TCP connection, ...) sent over the network or anything else that is not supposed to be serializable (that's the reason why I didn't change a single line of code in AerospikeClient.java code)

Beside testing use case, in a real use case, we have been sending the client factory (configuration parameters which are serializable) to the worker machine, which can create a concrete instance of the client accessible by multiple threads on the same machine.

Aloren commented 6 years ago

In the first comment you showed code with serialization of IAerospikeClient. What is the purpose of this test? To mock client it doesn’t need to be serializable.

fpopic commented 6 years ago

This code just proofs correctness of my pull request, that the client's interface will be serializable with all it's dependencies that are being imported.

Aloren commented 6 years ago

What is the purpose of making client serializable? Client can be mocked without need to be marked with Serializable interface.

BrianNichols commented 6 years ago

@fpopic One problem with making async listener interfaces (like RecordListener) serializable is that user's application code that implements these interfaces are required to be serializable too. This cascading effect is undesirable. It also results in annoying warnings in eclipse that serialVersionUID is not declared.

Is there any way that you can implement remote mocking without these undesirable side effects?

fpopic commented 6 years ago

@BrianNichols

I tried but I think it is not possible with mocking libraries, because in tests you need to have a reference to the mocked client object to set the mock/stub behaviour (what mock returns when mock[IAerospikeClient].get(), mock[IAerospikeClient].put() is called), to verify how many times some method will be called and so on)

def workerJob(client: IAerospikeClient){
  do some job....
}

def test{
       val client = mock[IAerospikeClient]
       client.get(...).returns(....) // behaviour
       workerJob(client)
}

and if you pass only a configuration to create IAerospikeClient, a worker will be able to create the object, but then it is not possible to refer to it and set your expectations.

def workerJob2(host:String, port:Int, isTest: Bool){
  val client = if (isTest) mock[IAerospikeClient] 
              else new AerospikeClient(host, port)
  do some job....
}

def test2{
      workerJob("localhost", 3000) 
      // you can't put your expectations because you don't have the object!
}

and "remote mocking" will be even worse if you mean on this?

def workerJob3(host:String, port:Int, isTest: Bool){
  val client = if (isTest) mock[IAerospikeClient]
                 ... set some expectations here?
              else new AerospikeClient(host, port)
  do some job....
}

Because then you will have to code your mocked behaviour "test" code inside "src" code, which is an even worse code smell.

Maybe then the only "workaround" will be to use a custom implementation of IAerospikeClient that won't be serializable and will have some dummy in-memory objects and act as a mock?

def workerJob4(host:String, port:Int, isTest: Bool){
  val client = if (isTest) new IAerospikeClient() { def get(...){return ....}}
              else new AerospikeClient(host, port)
  do some job....
}
BrianNichols commented 6 years ago

Maybe then the only "workaround" will be to use a custom implementation of IAerospikeClient that won't be serializable and will have some dummy in-memory objects and act as a mock?

@fpopic Yes, if you can make that work, then we can skip the serialization part.