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

Making IAerospikeClient extend java.io.Serializable #91

Merged
merged 1 commit into from Feb 16, 2018
Merged

Making IAerospikeClient extend java.io.Serializable #91

merged 1 commit into from Feb 16, 2018

Conversation

fpopic
Copy link
Contributor

@fpopic fpopic commented Jan 27, 2018

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 #90

… all dependencies (only supertypes) that have been used by the interface.
@fpopic
Copy link
Contributor Author

fpopic commented Jan 29, 2018

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
Copy link
Member

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

@fpopic
Copy link
Contributor Author

fpopic commented Jan 30, 2018

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

@fpopic fpopic closed this Jan 30, 2018
@fpopic fpopic reopened this Jan 30, 2018
@Aloren
Copy link
Contributor

Aloren commented Feb 8, 2018

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

@BrianNichols
Copy link
Member

Good question. What are your arguments against it?

@Aloren
Copy link
Contributor

Aloren commented Feb 8, 2018

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
Copy link
Contributor

Aloren commented Feb 8, 2018

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. 😕

@BrianNichols
Copy link
Member

@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
Copy link
Contributor Author

fpopic commented Feb 9, 2018

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
Copy link
Contributor

Aloren commented Feb 9, 2018

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
Copy link
Contributor Author

fpopic commented Feb 10, 2018

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
Copy link
Contributor

Aloren commented Feb 10, 2018

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

@BrianNichols
Copy link
Member

@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
Copy link
Contributor Author

fpopic commented Feb 15, 2018

@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
Copy link
Member

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.

@BrianNichols BrianNichols merged commit 6c77af2 into aerospike:master Feb 16, 2018
ijusti pushed a commit to ijusti/aerospike-client-java that referenced this pull request Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants