You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "vldpyatkov (via GitHub)" <gi...@apache.org> on 2024/12/04 18:54:25 UTC
[PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
vldpyatkov opened a new pull request, #4835:
URL: https://github.com/apache/ignite-3/pull/4835
https://issues.apache.org/jira/browse/IGNITE-23837
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1883427161
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -282,6 +298,14 @@ private LockState lockState(LockKey key) {
if (v == null) {
res[0] = slots[index];
assert !res[0].markedForRemove;
+
+ long currentTs = coarseCurrentTimeMillis();
+
+ long previousTs = lastTsPintWarn.get();
+
+ if ((currentTs - previousTs) > warningPrintInterval && lastTsPintWarn.compareAndSet(previousTs, currentTs)) {
Review Comment:
I'd expect that we will release some locks on conflicts and thus cleanup some slots.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882383242
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -282,6 +298,14 @@ private LockState lockState(LockKey key) {
if (v == null) {
res[0] = slots[index];
assert !res[0].markedForRemove;
+
+ long currentTs = coarseCurrentTimeMillis();
+
+ long previousTs = lastTsPintWarn.get();
+
+ if ((currentTs - previousTs) > warningPrintInterval && lastTsPintWarn.compareAndSet(previousTs, currentTs)) {
Review Comment:
Why did you implement it that way? I was expecting that you will log warnin every time when log manager runs out of slots. And when some slots will be released you will reset printWarn flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882541226
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
4. What's the difference between clearInlineMocks and clearAllCaches? Why do we need to call both?
Got it. You've answered it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1877573746
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -282,6 +292,10 @@ private LockState lockState(LockKey key) {
if (v == null) {
res[0] = slots[index];
assert !res[0].markedForRemove;
+
+ if (isSlotsExhausted.compareAndExchange(false, true)) {
Review Comment:
Where do you reset isSlotsExhausted back to false?
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
1. What's the rule of thumb to define wether clearAllCaches is required or not?
2. Is it required to call it in AfterEach or AfterAll will also do the trick?
3. What's the disadvantage of calling clearAllCaches()? In other words, why not to call it in AfterAll within every base test. E.g. we may call it BaseIgniteAbstractTest.
4. What's the difference between clearInlineMocks and clearAllCaches? Why do we need to call both?
##########
modules/client/src/test/java/org/apache/ignite/client/fakes/FakeIgniteTables.java:
##########
@@ -246,7 +246,7 @@ public CompletableFuture<Integer> schemaVersionAtCurrentTime(int tableId) {
}
private static LockManager lockManager() {
- HeapLockManager lockManager = new HeapLockManager();
+ HeapLockManager lockManager = new HeapLockManager(1024, 1024);
Review Comment:
What's the point in decreasing slots size here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1883412757
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
Still not clear. Let's discuss it after the morning sync.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882538892
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -282,6 +292,10 @@ private LockState lockState(LockKey key) {
if (v == null) {
res[0] = slots[index];
assert !res[0].markedForRemove;
+
+ if (isSlotsExhausted.compareAndExchange(false, true)) {
Review Comment:
Why didn't you reset back the flag on slots release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882482253
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -282,6 +298,14 @@ private LockState lockState(LockKey key) {
if (v == null) {
res[0] = slots[index];
assert !res[0].markedForRemove;
+
+ long currentTs = coarseCurrentTimeMillis();
+
+ long previousTs = lastTsPintWarn.get();
+
+ if ((currentTs - previousTs) > warningPrintInterval && lastTsPintWarn.compareAndSet(previousTs, currentTs)) {
Review Comment:
If we do not pay attention to the enormous amount of locks, we will be able to see the log file. But it might not be possible if we log it only once and the file is rotated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882540351
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
3. What's the disadvantage of calling clearAllCaches()? In other words, why not to call it in AfterAll within every base test. E.g. we may call it BaseIgniteAbstractTest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1883631151
##########
modules/transactions/src/test/java/org/apache/ignite/internal/tx/TxManagerTest.java:
##########
@@ -184,6 +183,8 @@ public void tearDown() {
txManager.beforeNodeStop();
assertThat(txManager.stopAsync(new ComponentContext()), willCompleteSuccessfully());
+
+ MockUtil.clearAllCaches();
Review Comment:
clearInlineCaches?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1877590872
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -282,6 +292,10 @@ private LockState lockState(LockKey key) {
if (v == null) {
res[0] = slots[index];
assert !res[0].markedForRemove;
+
+ if (isSlotsExhausted.compareAndExchange(false, true)) {
Review Comment:
Only the first time we are logging it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1880160836
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -282,6 +292,10 @@ private LockState lockState(LockKey key) {
if (v == null) {
res[0] = slots[index];
assert !res[0].markedForRemove;
+
+ if (isSlotsExhausted.compareAndExchange(false, true)) {
Review Comment:
I made the output through the interval.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882371945
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
The description of https://issues.apache.org/jira/browse/IGNITE-23956 seems a bit messy. Do you mean that besides clearInlineMocks() it's also required to call clearAllCaches within @AfterAll?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1880162650
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
https://issues.apache.org/jira/browse/IGNITE-23956
Described the issue here and added TODOs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882539507
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
1. What's the rule of thumb to define wether clearAllCaches is required or not?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882539877
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
2. Is it required to call it in AfterEach or AfterAll will also do the trick?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1883412757
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
Still not clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882600336
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
By questions:
1. Invoke clearInlineMocks() after any test (in the case were the mockito do not use the method do nothing), is case where we are not going to use created mocks in the next test.
2. Depends on scenario, look at poit 1.
3. Many test use MockitoExtension. The extention manages mocks independently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1877593593
##########
modules/client/src/test/java/org/apache/ignite/client/fakes/FakeIgniteTables.java:
##########
@@ -246,7 +246,7 @@ public CompletableFuture<Integer> schemaVersionAtCurrentTime(int tableId) {
}
private static LockManager lockManager() {
- HeapLockManager lockManager = new HeapLockManager();
+ HeapLockManager lockManager = new HeapLockManager(1024, 1024);
Review Comment:
Because it is not needed to make it more than 1K slots. Just to save the memory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1877672366
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/HeapLockManager.java:
##########
@@ -282,6 +292,10 @@ private LockState lockState(LockKey key) {
if (v == null) {
res[0] = slots[index];
assert !res[0].markedForRemove;
+
+ if (isSlotsExhausted.compareAndExchange(false, true)) {
Review Comment:
What do you mean? On first logging it's updated from false to true. I'm asking about resetting back from true to false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882386629
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
Anyway, all questions above still lacks answers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835#discussion_r1882396262
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceDistributedTest.java:
##########
@@ -369,6 +370,8 @@ void before(TestInfo testInfo) throws Exception {
@AfterEach
void after() {
nodes.forEach(Node::stop);
+
+ MockUtil.clearAllCaches();
Review Comment:
clearAllCaches() includes clearInlineMocks(). The last one is necessary to avoid leak,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23837 Add log when lock manager run out of slots [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov merged PR #4835:
URL: https://github.com/apache/ignite-3/pull/4835
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org