hazelcast-cpp-client icon indicating copy to clipboard operation
hazelcast-cpp-client copied to clipboard

Assertion Failure is Fixed [API-1501]

Open hakanaktas0 opened this issue 2 years ago • 13 comments

solves this issue

hakanaktas0 avatar Aug 23 '22 12:08 hakanaktas0

Windows test FAILed.

devOpsHazelcast avatar Aug 23 '22 15:08 devOpsHazelcast

Linux test FAILed.

devOpsHazelcast avatar Aug 23 '22 15:08 devOpsHazelcast

verify

ihsandemir avatar Aug 25 '22 15:08 ihsandemir

verify

hakanaktas0 avatar Aug 25 '22 15:08 hakanaktas0

Windows test PASSed.

devOpsHazelcast avatar Aug 25 '22 16:08 devOpsHazelcast

Linux test PASSed.

devOpsHazelcast avatar Aug 25 '22 16:08 devOpsHazelcast

Windows test PASSed.

devOpsHazelcast avatar Aug 25 '22 16:08 devOpsHazelcast

Linux test PASSed.

devOpsHazelcast avatar Aug 25 '22 16:08 devOpsHazelcast

Because TTL has to be an integer it can't be less than 1, 0 is infinite, I had to add 1 second sleep. Before the fix, this test would give an assertion error, should pass without problems after the fix.

hakanaktas0 avatar Aug 30 '22 17:08 hakanaktas0

Windows test FAILed.

devOpsHazelcast avatar Aug 30 '22 17:08 devOpsHazelcast

Linux test PASSed.

devOpsHazelcast avatar Aug 30 '22 18:08 devOpsHazelcast

Windows test PASSed.

devOpsHazelcast avatar Aug 30 '22 19:08 devOpsHazelcast

Linux test PASSed.

devOpsHazelcast avatar Aug 30 '22 19:08 devOpsHazelcast

Windows test PASSed.

devOpsHazelcast avatar Sep 12 '22 11:09 devOpsHazelcast

Linux test PASSed.

devOpsHazelcast avatar Sep 12 '22 12:09 devOpsHazelcast

I have reviewev the PR and here are my findings :

  • Firstly, the fix is correct so it works.

But I don't think it is a good implementation because we call emplace function 3 times. Indeed, there is no need for that, it can be handled with only one emplace operation.

  1. Add copy assignment special member function to the session_state class.
// cp_impl.h#96
session_state& operator=(const session_state& rhs);
  1. Make definition of it as :
// cp_impl.cpp
proxy_session_manager::session_state&
proxy_session_manager::session_state::operator=(const session_state& rhs)
{
  id  = rhs.id;
  ttl = rhs.ttl;
  creation_time = rhs.creation_time;
  acquire_count = rhs.acquire_count.load();

  return *this;
}
  1. Define create_new_session like this. If there is already an entry at the sessions_ with the specified group_id, then just assign brand-new session_state to the value of it.
   // the lock_ is already acquired as write lock
    auto response = request_new_session(group_id);
    session_state state { response.id , response.ttl_millis };

    auto result = sessions_.emplace( group_id, state );
    
    if(!result.second)
        result.first->second = state;

    schedule_heartbeat_task(response.heartbeat_millis);
    return result.first;

OzanCansel avatar Oct 28 '22 06:10 OzanCansel