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

Fixes issues with ObjectDataInput/Output public API #16064

Merged
merged 1 commit into from Nov 21, 2019

Conversation

@vbekiaris
Copy link
Contributor

vbekiaris commented Nov 19, 2019

  • Do not expose Data in ObjectDataOutput/Input public API
  • Same as above for SerializationService in ObjectDataInput

EE counterpart: hazelcast/hazelcast-enterprise#3373

@vbekiaris vbekiaris self-assigned this Nov 19, 2019
@vbekiaris vbekiaris marked this pull request as ready for review Nov 19, 2019
@vbekiaris vbekiaris changed the title Fixes issues with ObjectDataInput/Output public API [DO NOT MERGE] Fixes issues with ObjectDataInput/Output public API Nov 19, 2019
@sancar
sancar approved these changes Nov 20, 2019
@vbekiaris vbekiaris changed the title [DO NOT MERGE] Fixes issues with ObjectDataInput/Output public API Fixes issues with ObjectDataInput/Output public API Nov 20, 2019
@vbekiaris vbekiaris requested a review from ahmetmircik Nov 20, 2019
@@ -28,7 +28,7 @@
* @see CacheEntryView
*/
@FunctionalInterface
public interface CacheEvictionPolicyComparator<K, V>
public interface CacheEvictionPolicyComparator<K, V>

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Nov 20, 2019

Member

indentation?

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Nov 21, 2019

Author Contributor

fixed


import java.io.IOException;

public interface DataReader {

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Nov 20, 2019

Member

can we have a short javaDoc to tell reasoning behind introducing this interface?

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Nov 21, 2019

Author Contributor

Added. Since this interface only makes sense as an extension of DataInput (already extended by ObjectDataInput), now DataReader extends DataInput.


import java.io.IOException;

public interface DataWriter {

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Nov 20, 2019

Member

can we have a short javaDoc to tell reasoning behind introducing this interface?

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Nov 21, 2019

Author Contributor

Same as above

}
return in.readObject();
}

public static void writeData(ObjectDataOutput out, Data data) throws IOException {
assert out instanceof DataWriter : "out must be an instance of DataWriter";

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Nov 20, 2019

Member

can there be any performance cost of assertions even we disable it? Since before this PR, we have no assertions inside writeData.

This comment has been minimized.

Copy link
@vbekiaris

vbekiaris Nov 21, 2019

Author Contributor

Did a benchmark performing serializationService.toData() on an EntryEventData instance with Data key & value (200 bytes long) that shows no slow down:

without assertions (commit ec5f2f6)
CompletableFutureBenchmark.assertionCost  thrpt   50  2940496,668 ± 175423,573  ops/s

with assertions in writeData, assertions not enabled in JVM
CompletableFutureBenchmark.assertionCost  thrpt   50  3112858,606 ± 165626,511  ops/s
Copy link
Member

ahmetmircik left a comment

minor comments + LGTM

Also removes SerializationService from ObjectDataInput public API
@vbekiaris vbekiaris force-pushed the vbekiaris:fixes/4.0/private-data branch from e3daec7 to 274d006 Nov 21, 2019
@vbekiaris vbekiaris merged commit 7651c81 into hazelcast:master Nov 21, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@vbekiaris vbekiaris deleted the vbekiaris:fixes/4.0/private-data branch Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.