Uploaded image for project: 'Ignite'
  1. Ignite
  2. IGNITE-3878

Add isPrimary() and isBackup() methods on CacheQUeryEntryEvent

    Details

    • Type: Improvement
    • Status: In Progress
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.7
    • Fix Version/s: 2.3
    • Component/s: cache
    • Labels:
      None

      Description

      In some cases useful know where (on primary or backup nodes) was invoked a continuous query filter.

        Issue Links

          Activity

          Hide
          dreamx Maksim Kozlov added a comment -

          Nikolay Tikhonov, if I understand correctly, do you want add these methods in CacheQueryEntryEvent and implement them in classes CacheContinuousQueryEvent and CacheEntryEventImpl?

          Show
          dreamx Maksim Kozlov added a comment - Nikolay Tikhonov , if I understand correctly, do you want add these methods in CacheQueryEntryEvent and implement them in classes CacheContinuousQueryEvent and CacheEntryEventImpl?
          Hide
          ntikhonov Nikolay Tikhonov added a comment -

          Maksim Kozlov Yes, you are right. Implementation should be similar on CacheQueryEntryEvent#getPartitionUpdateCounter.

          Show
          ntikhonov Nikolay Tikhonov added a comment - Maksim Kozlov Yes, you are right. Implementation should be similar on CacheQueryEntryEvent#getPartitionUpdateCounter .
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user dream-x opened a pull request:

          https://github.com/apache/ignite/pull/1393

          IGNITE-3878: Add isPrimary() and isBackup() methods on CacheQueryEntryEvent

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/dream-x/ignite ignite-3878

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/ignite/pull/1393.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #1393


          commit d20e42aa2417224b4a27af2f8525b098d4d99d60
          Author: Max Kozlov <dreamx.max@gmail.com>
          Date: 2016-12-28T08:33:52Z

          Add isPrimary() and isBackup() methods in CacheQueryEntryEvent

          commit 59a77bf7b40162cf4dc9ed234956347d1c98ab38
          Author: Max Kozlov <dreamx.max@gmail.com>
          Date: 2016-12-28T08:34:36Z

          Add implementation isPrimary() and isBackup() methods in CacheContinuousQueryEvent

          commit fad42f7b1e13c41cd36a344bb9de4a96ed1eb136
          Author: Max Kozlov <dreamx.max@gmail.com>
          Date: 2016-12-28T08:35:07Z

          Add implementation isPrimary() and isBackup() methods in CacheEntryEventImpl


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user dream-x opened a pull request: https://github.com/apache/ignite/pull/1393 IGNITE-3878 : Add isPrimary() and isBackup() methods on CacheQueryEntryEvent You can merge this pull request into a Git repository by running: $ git pull https://github.com/dream-x/ignite ignite-3878 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/ignite/pull/1393.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1393 commit d20e42aa2417224b4a27af2f8525b098d4d99d60 Author: Max Kozlov <dreamx.max@gmail.com> Date: 2016-12-28T08:33:52Z Add isPrimary() and isBackup() methods in CacheQueryEntryEvent commit 59a77bf7b40162cf4dc9ed234956347d1c98ab38 Author: Max Kozlov <dreamx.max@gmail.com> Date: 2016-12-28T08:34:36Z Add implementation isPrimary() and isBackup() methods in CacheContinuousQueryEvent commit fad42f7b1e13c41cd36a344bb9de4a96ed1eb136 Author: Max Kozlov <dreamx.max@gmail.com> Date: 2016-12-28T08:35:07Z Add implementation isPrimary() and isBackup() methods in CacheEntryEventImpl
          Hide
          dmagda Denis Magda added a comment -

          Maksim Kozlov, if you think the ticket is ready for review please change the status from "in progress" to "patch available". Also make sure you have added additional tests to check your functionality and validated the changes on Team City. Refer to this documentation section for more details
          https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-1.CreateGitHubpull-request

          Show
          dmagda Denis Magda added a comment - Maksim Kozlov , if you think the ticket is ready for review please change the status from "in progress" to "patch available". Also make sure you have added additional tests to check your functionality and validated the changes on Team City. Refer to this documentation section for more details https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-1.CreateGitHubpull-request
          Hide
          yzhdanov Yakov Zhdanov added a comment -

          I have posted some comments in the PR on github. Please fix and let me know.

          Show
          yzhdanov Yakov Zhdanov added a comment - I have posted some comments in the PR on github. Please fix and let me know.
          Hide
          dreamx Maksim Kozlov added a comment - - edited

          Yakov Zhdanov I'm fix this comment PR. What is meant by "more test cases", ex. add test which read data from backup node?

          Show
          dreamx Maksim Kozlov added a comment - - edited Yakov Zhdanov I'm fix this comment PR. What is meant by "more test cases", ex. add test which read data from backup node?
          Hide
          yzhdanov Yakov Zhdanov added a comment -

          Maksim Kozlov, sorry for the delay.

          Is it possible to create the test which can assert that isBackup returns true when entry is checked on backup?

          Please change javadoc to use tags, e.g.

          {@code True}

          Please fix these minor comments and I think we are done.

          Show
          yzhdanov Yakov Zhdanov added a comment - Maksim Kozlov , sorry for the delay. Is it possible to create the test which can assert that isBackup returns true when entry is checked on backup? Please change javadoc to use tags, e.g. {@code True} Please fix these minor comments and I think we are done.
          Hide
          yzhdanov Yakov Zhdanov added a comment -

          Maksim Kozlov did you try to add test that asserts isBackup() returns true when entry is checked on backup?

          Show
          yzhdanov Yakov Zhdanov added a comment - Maksim Kozlov did you try to add test that asserts isBackup() returns true when entry is checked on backup?
          Hide
          dreamx Maksim Kozlov added a comment -
          Show
          dreamx Maksim Kozlov added a comment - Yakov Zhdanov yes.
          Hide
          dreamx Maksim Kozlov added a comment -

          Yakov Zhdanov After reviewing the packet org.apache.ignite.internal.processors.cache.query.continuous I have not found a single case that I need. If I understand the logic of the backup node when the primary node reading will occur is not available (because any settings that I found do not allow the reader to pinpoint priority to backup, and I understand that this is not all you need , but may have something that I missed)?

          Show
          dreamx Maksim Kozlov added a comment - Yakov Zhdanov After reviewing the packet org.apache.ignite.internal.processors.cache.query.continuous I have not found a single case that I need. If I understand the logic of the backup node when the primary node reading will occur is not available (because any settings that I found do not allow the reader to pinpoint priority to backup, and I understand that this is not all you need , but may have something that I missed)?
          Hide
          yzhdanov Yakov Zhdanov added a comment -

          Nikolay Tikhonov, can you please join us and confirm that event is fired on backup as well, so event entry is available on backups as well and entry.isBackup() should return true?

          Show
          yzhdanov Yakov Zhdanov added a comment - Nikolay Tikhonov , can you please join us and confirm that event is fired on backup as well, so event entry is available on backups as well and entry.isBackup() should return true?
          Hide
          ntikhonov Nikolay Tikhonov added a comment -

          Yakov Zhdanov,
          Yes, continuous query filter triggered on primary and backup nodes.

          Maksim Kozlov,
          You can add remote filter in CQ and check that it will be invoked once on primary node (with isPrimary == true) and several times on backup nodes (with isBackup == true).

          Show
          ntikhonov Nikolay Tikhonov added a comment - Yakov Zhdanov , Yes, continuous query filter triggered on primary and backup nodes. Maksim Kozlov , You can add remote filter in CQ and check that it will be invoked once on primary node (with isPrimary == true) and several times on backup nodes (with isBackup == true).
          Hide
          dreamx Maksim Kozlov added a comment -

          Nikolay Tikhonov I added a check on which you wrote in the previous comments, all right? CacheContinuousQueryFailoverAbstractSelfTest class, testRemoteFilter method, commit.

          Show
          dreamx Maksim Kozlov added a comment - Nikolay Tikhonov I added a check on which you wrote in the previous comments, all right? CacheContinuousQueryFailoverAbstractSelfTest class, testRemoteFilter method, commit .
          Hide
          dreamx Maksim Kozlov added a comment -

          Yakov Zhdanov, Nikolay Tikhonov could you review the code (in previous comment), please.

          Show
          dreamx Maksim Kozlov added a comment - Yakov Zhdanov , Nikolay Tikhonov could you review the code (in previous comment), please.
          Hide
          yzhdanov Yakov Zhdanov added a comment -

          Maksim Kozlov, Nikolay Tikhonov, I see the following code

                      if (!asyncCallback()) {
                          assertEquals(1, lsnr.getCountPrimary());
                          assertTrue(lsnr.getCountBackup() > 1);
                      }
          

          and I don't get it. My understanding that listener should be called on primary and backups no matter sync or async mode is used. I removed this if and async notification tests started failing.

          Nikolay Tikhonov, you should work together with Maksim and refactor tests to get consistent checks.

          Show
          yzhdanov Yakov Zhdanov added a comment - Maksim Kozlov , Nikolay Tikhonov , I see the following code if (!asyncCallback()) { assertEquals(1, lsnr.getCountPrimary()); assertTrue(lsnr.getCountBackup() > 1); } and I don't get it. My understanding that listener should be called on primary and backups no matter sync or async mode is used. I removed this if and async notification tests started failing. Nikolay Tikhonov , you should work together with Maksim and refactor tests to get consistent checks.
          Hide
          dreamx Maksim Kozlov added a comment - - edited

          Yakov Zhdanov when creating final CacheEventListener3 lsnr = asyncCallback() ? new CacheEventAsyncListener3() : new CacheEventListener3(); and asyncCallback() is true then instance CacheEventAsyncListener3 implementation is No-op, so I decided that we should not check in this version.

          Show
          dreamx Maksim Kozlov added a comment - - edited Yakov Zhdanov when creating final CacheEventListener3 lsnr = asyncCallback() ? new CacheEventAsyncListener3() : new CacheEventListener3(); and asyncCallback() is true then instance CacheEventAsyncListener3 implementation is No-op , so I decided that we should not check in this version.
          Hide
          ntikhonov Nikolay Tikhonov added a comment -

          Maksim Kozlov
          Thank you for your contribution. We glad see you in Apache Ignite community.
          CacheEntryAsyncListener3 has @IgniteAsyncCallback annotation due this listeners have different semantics (more details see javadoc for the annotation) and this case should be tested. Let's refactored this test that it works for all cases (without any if).
          Also your changes don't work correctly for filter. For each an update, filter invoked on primary and backup nodes. In current implementation CacheQueryEntryEvent#isBackup method for this cases always return false. Need to fix it and add tests on it.

          Show
          ntikhonov Nikolay Tikhonov added a comment - Maksim Kozlov Thank you for your contribution. We glad see you in Apache Ignite community. CacheEntryAsyncListener3 has @IgniteAsyncCallback annotation due this listeners have different semantics (more details see javadoc for the annotation) and this case should be tested. Let's refactored this test that it works for all cases (without any if ). Also your changes don't work correctly for filter. For each an update, filter invoked on primary and backup nodes. In current implementation CacheQueryEntryEvent#isBackup method for this cases always return false . Need to fix it and add tests on it.
          Hide
          ntikhonov Nikolay Tikhonov added a comment -

          Also, please, pay attention that cntPrimary and cntBackup variables can be updated from many threads. Better way using atomic primitive from java.util.concurrent.

          Show
          ntikhonov Nikolay Tikhonov added a comment - Also, please, pay attention that cntPrimary and cntBackup variables can be updated from many threads. Better way using atomic primitive from java.util.concurrent.
          Hide
          dreamx Maksim Kozlov added a comment - - edited

          Nikolai Tikhonov Please review. TeamCity

          Show
          dreamx Maksim Kozlov added a comment - - edited Nikolai Tikhonov Please review. TeamCity

            People

            • Assignee:
              dreamx Maksim Kozlov
              Reporter:
              ntikhonov Nikolay Tikhonov
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:

                Development