Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature - reset cursor on Reader to current position #4331

Merged

Conversation

lovelle
Copy link
Contributor

@lovelle lovelle commented May 21, 2019

Motivation

There are some cases in which is it useful to be able to include current
position of message when reset of cursor was made.

This was reported by a vvy on slack, no issue has been created to track this.

Modifications

  • Add startMessageIdInclusive() to support include current position of
    reset on ReaderBuilder.
  • Add resetIncludeHead field for Reader and Consumer Configuration Data
  • Fix position of cursor for non durable consumer.
  • Improve discard if statement for batch enable mode.
  • Add discard if statement for batch disable mode.
  • Add test case to assert the start of specific message id at the expected
    position with data provider scenarios:
    A. Batch enable and start inclusive enable.
    B. Batch enable and start inclusive disable.
    C. Batch disable and start inclusive enable.
    D. Batch disable and start inclusive disable.

@jiazhai
Copy link
Member

jiazhai commented May 22, 2019

run java8 tests

@jiazhai
Copy link
Member

jiazhai commented May 22, 2019

lgtm. Indeed there is needs for this case.

@lovelle lovelle force-pushed the bugfix/dont_discard_first_entry_after_reset branch from 79b6f07 to 4fa8153 Compare May 22, 2019 15:30
@lovelle lovelle changed the title Bugfix - don't discard first entry after reset Feature - reset cursor on Reader to current position May 22, 2019
@merlimat merlimat added the type/feature The PR added a new feature or issue requested a new feature label May 22, 2019
@merlimat merlimat added this to the 2.4.0 milestone May 22, 2019
@lovelle lovelle force-pushed the bugfix/dont_discard_first_entry_after_reset branch from 4fa8153 to 9e62a97 Compare May 22, 2019 20:14
@lovelle lovelle force-pushed the bugfix/dont_discard_first_entry_after_reset branch from 9e62a97 to 30584b1 Compare May 24, 2019 14:54
@jiazhai
Copy link
Member

jiazhai commented May 25, 2019

@lovelle As @merlimat suggested, Would you please also help add a test parameter for batch/non-batch, this is a setting for producer config. So both batch and non-batch could be covered.

@lovelle lovelle force-pushed the bugfix/dont_discard_first_entry_after_reset branch from 30584b1 to 3f0d66f Compare May 25, 2019 15:02
@lovelle
Copy link
Contributor Author

lovelle commented May 25, 2019

@jiazhai Of course, I've just added, please take a look now, sorry for delay.

@jiazhai
Copy link
Member

jiazhai commented May 27, 2019

run cpp tests
run integration tests

@lovelle lovelle force-pushed the bugfix/dont_discard_first_entry_after_reset branch from 3f0d66f to 66b0f04 Compare May 27, 2019 03:15
@lovelle lovelle force-pushed the bugfix/dont_discard_first_entry_after_reset branch 2 times, most recently from f75dc13 to a6f6cd1 Compare May 28, 2019 11:53
@lovelle
Copy link
Contributor Author

lovelle commented May 30, 2019

run cpp tests

1 similar comment
@lovelle
Copy link
Contributor Author

lovelle commented May 30, 2019

run cpp tests

@lovelle
Copy link
Contributor Author

lovelle commented May 30, 2019

@jiazhai cpp tests keeps failing without notice of any failed tests, any hint on this? keep trying?

@lovelle
Copy link
Contributor Author

lovelle commented May 31, 2019

run cpp tests

*Motivation*

There are some cases in which is it useful to be able to include current
position of message when reset of cursor was made.

This was reported by a `vvy` on slack, no issue has been created to track this.

*Modifications*

  - Add startMessageIdInclusive() to support include current position of
    reset on ReaderBuilder.
  - Add resetIncludeHead field for Reader and Consumer Configuration Data
  - Fix position of cursor for non durable consumer.
  - Improve discard if statement for batch enable mode.
  - Add discard if statement for batch disable mode.
  - Improve test case for latest Reader seek.
  - Add test case to assert the start of specific message id at the expected
    position with data provider scenarios:
      A. Batch enable and start inclusive enable.
      B. Batch enable and start inclusive disable.
      C. Batch disable and start inclusive enable.
      D. Batch disable and start inclusive disable.
@lovelle lovelle force-pushed the bugfix/dont_discard_first_entry_after_reset branch from ed8c5c7 to 91055f0 Compare May 31, 2019 16:07
@jiazhai
Copy link
Member

jiazhai commented Jun 1, 2019

run cpp tests
run java8 tests

@jiazhai
Copy link
Member

jiazhai commented Jun 1, 2019

@lovelle right, seems no error in cpp, just timeout.

[141/153]Build timed out (after 200 minutes). Marking the build as aborted.

@jiazhai
Copy link
Member

jiazhai commented Jun 2, 2019

run cpp tests

@yjshen
Copy link
Member

yjshen commented Jun 3, 2019

Python test failed:

2019-06-02 07:15:51.452 INFO  ConsumerImpl:170 | [persistent://public/default/my-string-python-topic, sub-1, 0] Created consumer on broker [127.0.0.1:48310 -> 127.0.0.1:6650] 
2019-06-02 07:15:51.473 INFO  ClientImpl:479 | Closing Pulsar client
2019-06-02 07:15:51.474 INFO  ProducerImpl:475 | [persistent://public/default/my-string-python-topic, standalone-0-219] Closing producer for topic persistent://public/default/my-string-python-topic
2019-06-02 07:15:51.474 INFO  ProducerImpl:475 | [persistent://public/default/my-string-python-topic, standalone-0-220] Closing producer for topic persistent://public/default/my-string-python-topic
2019-06-02 07:15:51.474 INFO  ConsumerImpl:839 | [persistent://public/default/my-string-python-topic, sub-1, 0] Closing consumer for topic persistent://public/default/my-string-python-topic
2019-06-02 07:15:51.474 INFO  ProducerImpl:513 | [persistent://public/default/my-string-python-topic, standalone-0-219] Closed producer
2019-06-02 07:15:51.474 INFO  ProducerImpl:513 | [persistent://public/default/my-string-python-topic, standalone-0-220] Closed producer
2019-06-02 07:15:51.475 INFO  ConsumerImpl:875 | [persistent://public/default/my-string-python-topic, sub-1, 0] Closed consumer 0
.
======================================================================
FAIL: test_reader_on_specific_message (__main__.PulsarTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "pulsar_test.py", line 395, in test_reader_on_specific_message
    self.assertEqual(msg.data(), b'hello-%d' % i)
AssertionError: 'hello-4' != 'hello-5'

----------------------------------------------------------------------
Ran 51 tests in 52.234s

@lovelle
Copy link
Contributor Author

lovelle commented Jun 5, 2019

Ok, now I found it, let me explore this a little bit further

@sijie
Copy link
Member

sijie commented Jun 9, 2019

@lovelle I moved this to 2.5.0. if you think it should be 2.4.0, please let me know.

@sijie sijie modified the milestones: 2.4.0, 2.5.0 Jun 9, 2019
@jiazhai
Copy link
Member

jiazhai commented Jun 10, 2019

run cpp tests

@yjshen
Copy link
Member

yjshen commented Jun 10, 2019

======================================================================
FAIL: test_reader_on_specific_message (__main__.PulsarTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "pulsar_test.py", line 403, in test_reader_on_specific_message
    self.assertEqual(msg.data(), b'hello-%d' % i)
AssertionError: 'hello-4' != 'hello-5'

----------------------------------------------------------------------
Ran 51 tests in 51.260s

Same error?

@lovelle lovelle force-pushed the bugfix/dont_discard_first_entry_after_reset branch 7 times, most recently from 00d47c7 to 863c0f3 Compare June 11, 2019 01:42
@lovelle lovelle force-pushed the bugfix/dont_discard_first_entry_after_reset branch from 863c0f3 to b9f4c03 Compare June 11, 2019 07:28
@lovelle
Copy link
Contributor Author

lovelle commented Jun 11, 2019

run java8 tests

@lovelle
Copy link
Contributor Author

lovelle commented Jun 11, 2019

@jiazhai @yjshen now is fixed, sorry for the trouble.

@sijie sure, no problem.

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@sijie
Copy link
Member

sijie commented Jun 13, 2019

@jiazhai @yjshen can you check this pull request again since you left comments before?

@yjshen
Copy link
Member

yjshen commented Jun 14, 2019

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants