Performance: PartitionClientRequest inefficient determining partition-id #4940

Closed
pveentjer opened this Issue Mar 30, 2015 · 8 comments

Projects

None yet

4 participants

@pveentjer
Member

ClientRequests are very inefficient determining the partition id.

E.g. in case of the AbstractAlterRequest:

 @Override
    protected int getPartition() {
        Data key = serializationService.toData(name);
        return getClientEngine().getPartitionService().getPartitionId(key);
    }

But the partition-id is passed to the member as part of the Packet and with the introduction of the client-protocol, it is found here as well. So we should make use of the pre-calculated partitionid instead of calculating it.

The new client protocol also has the partition-id available, see: ClientMessage 'partitionId' method.

So probably we can get rid of the 'getPartition' method completely.

@asimarslan WDYT.

Are you already passing in the partition-id on the client protocol? Or should this PR be parked?

@pveentjer pveentjer added this to the 3.6 milestone Mar 30, 2015
@pveentjer pveentjer changed the title from Performance: client requests inefficient determining partition-id to Performance: PartitionClientRequest inefficient determining partition-id Mar 30, 2015
@sancar sancar added the Team: Client label Mar 30, 2015
@sancar
Member
sancar commented Mar 30, 2015

Guys, can you please be more careful about adding related labels to issues and pull requests ?

@asimarslan
Member

@pveentjer ,
New client protocol has a partition-id field in the header for all messages and server side will use it and there won't be any re-calculation. Any code that re-calculate you see in new module is due to refactoring and will be removed.

@pveentjer
Member

@sancar what makes you think the labels are not placed correctly . The QuSP team probably is going to pick this up since this is a performance concern.

@pveentjer
Member

@asimarslan the Request object on the serverside will be removed once the client protocol is in place?

If it isn't going to be removed, can I already use the 'partitionId' method (with preculculated partitionid) instead of 'getPartitionId' with the reculculation of the partition-id.

@sancar
Member
sancar commented Mar 30, 2015

@pveentjer This is both Qusp team and Client team concern, both team should review and and share their ideas and concerns. Hence both client and qusp label should be there.
Note: I added the client label, it was not there when this issue first opened. This is why I felt like warning.I have no problem with qusp label.

@asimarslan
Member

@pveentjer ,

We are totally removing requests into messageHandlingTask which is the processing part of each request.
But we didn't coded yet. So you are optimising something not even coded :)
I can say that we won't calculate partition-id on server side. It will be more clear for you in my next PR in a day or so.

@bwzhang2011

@asimarslan, well done andd look forward to that not long in the future. projects with hazelcast integration heavily rely on hazelcast client module so it is with its performance and stability.

@sancar
Member
sancar commented May 20, 2015

Fixed with new client protocol. Partition id is calculated once and set to clientMessage. ClientMessage is the request going from server to client. And it is accessed from client message(not calculated again) on server side.

@Override
    protected int getPartition() {
        Data key = serializationService.toData(name);
        return getClientEngine().getPartitionService().getPartitionId(key);
    }

Equivalent method of the code above is here:

https://github.com/hazelcast/hazelcast/blob/master/hazelcast/src/main/java/com/hazelcast/client/impl/protocol/task/AbstractMessageTask.java#L83

Closing the issue.

@sancar sancar closed this May 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment