Skip to content

Conversation

@mdumandag
Copy link
Contributor

Implementation, tests, documentation and code samples for both the
session aware and sessionless semaphore proxies are added.

Also, another test dependency to parameterize tests is added.

Implementation, tests, documentation and code samples for both the
session aware and sessionless semaphore proxies are added.

Also, another test dependency to parameterize tests is added.
@puzpuzpuz puzpuzpuz self-requested a review November 3, 2020 09:48
"from the same thread." % self._object_name)
raise error
except Exception as e:
self._release_session(session_id)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this release session logic in Java client's SessionAwareSemaphoreProxy. Is it a leftover? If so, it makes sense to submit a PR to fix it in Java client and apply the change to other clients as well.

The same question applies to other methods with except Exception as e clause, like _do_drain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not leftovers, I forgot to send a PR to the Java side. I will do that and share the links within the team channel so that it can be applied to other clients. I also found that I was removing only 1 permit in case of an unspecified error on _do_acquire. Also fixed that and updated the test

@puzpuzpuz puzpuzpuz self-requested a review November 5, 2020 07:51
Copy link

@puzpuzpuz puzpuzpuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mdumandag mdumandag merged commit fec71a4 into hazelcast:master Nov 5, 2020
@mdumandag mdumandag deleted the cp-semaphore branch November 5, 2020 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants