Minor Response Packet Handler cleanup

[pveentjer]

Renamed BacicResponseHandler to BasicPacketResponseHandler to prevent name confusing since we already have ResponseHandler for different purpose.

BasicOperationScheduler ResponseThread is now explicitly making use of Packet; code was too generic for no reason. The queue was for packets, but there was still an unwanted check done for non packets from that queue.

[gurbuzali]

👍

[sancar]

My comments are not related to this pr but rather more general concern.

This ResponseThread seems awkward. Its process method is called directly. The thread itself waiting on a workingQueue that nobody putting anything into. It seems that packet deserialization is done in IO thread. Are we ok with these?

[pveentjer]

Hi Sancar.

I don't think that the the case. The IO thread takes a packet of the wire, checks if response+operation bit are set, and puts it on the queue of the response-thread. Then the response-thread takes it of the queue, deserializes it and then processes it.

So the IO thread is not doing any heavy lifting.

[pveentjer]

PS: I'm still working on getting rid of the response thread. I have made several attempts but tilll so far have not been able to improve performance.

[sancar]

Yes, the scenario that you mentioned what should have been. But I am afraid it is not the case. Just take a close look of workQueue of ResponseThread. I checked usages of workqueue, there is "take" method called on it, but nothing is pushing into it.

[pveentjer]

    public void execute(Packet packet) {
        try {
            if (packet.isHeaderSet(Packet.HEADER_RESPONSE)) {
                //it is an response packet.
                responseThread.process(packet);
            } else {
                //it is an must be an operation packet
                int partitionId = packet.getPartitionId();
                boolean hasPriority = packet.isUrgent();
                execute(packet, partitionId, hasPriority);
            }
        } catch (RejectedExecutionException e) {
            if (node.nodeEngine.isActive()) {
                throw e;
            }
        }
    }
``

[sancar]

Yes, the process is directly called, rather than putting into queue. Isnt that wrong? or am I missing something big? or You need coffee again?

[pveentjer]

It is an optimization we have been playing with, but it should never have made it in the master. I see I'm the cause of this change. The only way to get rid of the response queue is to get rid of the response object because they are now being deserialized on the IO thread.

[pveentjer]

Good spotting. I'll revert to changes in a different PR and see what the performance implications are. I'll do a bit more investigation what happened.

[pveentjer]

#4453

[pveentjer]

@gurbuzali @sancar can you have another look?

[sancar]

👍