You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "heesung-sn (via GitHub)" <gi...@apache.org> on 2023/04/07 22:12:59 UTC
[GitHub] [pulsar] heesung-sn opened a new pull request, #20044: [improve][client] Add isInterrupted func on TableView
heesung-sn opened a new pull request, #20044:
URL: https://github.com/apache/pulsar/pull/20044
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://pulsar.apache.org/contribute/develop-semantic-title/)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
<!-- Either this PR fixes an issue, -->
<!-- or this PR is one task of an issue -->
<!-- If the PR belongs to a PIP, please add the PIP link here -->
<!-- Details of when a PIP is required and how the PIP process work, please see: https://github.com/apache/pulsar/blob/master/wiki/proposals/PIP.md -->
### Motivation
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
If the tableview is interrupted, soon it will have outdated data as it stops reading more messages. In this case, callers need to know if the tableview is interrupted or not.
### Modifications
<!-- Describe the modifications you've done. -->
Added `booelan isInterrupted` func on TableView.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- Added a unit test.
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [x] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: skipped as this is a simple change.
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #20044: [improve][client] Add isInterrupted func on TableView
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #20044:
URL: https://github.com/apache/pulsar/pull/20044#issuecomment-1500813602
For the option 3, by this pr, callers could create new tableviews, by constantly monitoring if the table view is interrupted 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #20044: [improve][client] Add isInterrupted func on TableView
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #20044:
URL: https://github.com/apache/pulsar/pull/20044#issuecomment-1500947438
To expand this discussion a little further, I think the current tableview works under best-effort delivery and needs some improvement in the following areas for mission-critical data replication in production.
- guarantee liveness(better error handling. e.g infinite retry)
- handle slowness (how to handle slow consumer, e.g self-kill if too slow. If the consumer barely receives messages due to bugs, network, and etc.)
- expose operational metrics(table view state(retrying, active), consume latency, highest messageId(highest logical sequence number))
Currently, it is hard or impossible for callers to check them outside of tableview. And this pr is trying to expose the worst-case tableview state, where tableview is interrupted and stopped to consume messages. We could change the func name to isAlive, isConnected, isClosed, isStopped, or other. I found isInterrupted is a good choice here, as it tells if tableview is "interrupted" by some unknown reason.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #20044: [improve][client] Add retry when readTailMessages fails
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat merged PR #20044:
URL: https://github.com/apache/pulsar/pull/20044
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #20044: [improve][client] Add retry when readTailMessages fails
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #20044:
URL: https://github.com/apache/pulsar/pull/20044#issuecomment-1500963337
I updated the code to retry upon failure. @lifepuzzlefun
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #20044: [improve][client] Add retry when readTailMessages fails
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #20044:
URL: https://github.com/apache/pulsar/pull/20044#issuecomment-1500964944
@lifepuzzlefun I updated the code to "retry-forever" upon `readTailMessages` failure. I assume the current "fail-stop" is a bug.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #20044: [improve][client] Add isInterrupted func on TableView
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #20044:
URL: https://github.com/apache/pulsar/pull/20044#issuecomment-1500813083
1. retry forever upon failures.
2. throw RuntimeException from the data accessor apis, if already interrupted.(get, entrySet, keySet)
3. expose table view state(e.g isInterrupted)
It appears that we have the above options.
If tableview is supposed to guarantee the liveness, I think we can go to the option 1 instead.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codecov-commenter commented on pull request #20044: [improve][client] Add retry when readTailMessages fails
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20044:
URL: https://github.com/apache/pulsar/pull/20044#issuecomment-1501976474
## [Codecov](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20044](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (85cb28e) into [master](https://codecov.io/gh/apache/pulsar/commit/3b118b6bd54cd8e592b8c2cd329a3fa3dfd18a11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3b118b6) will **increase** coverage by `0.05%`.
> The diff coverage is `44.85%`.
[](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #20044 +/- ##
============================================
+ Coverage 72.86% 72.91% +0.05%
- Complexity 31628 31676 +48
============================================
Files 1861 1858 -3
Lines 137500 137518 +18
Branches 15141 15124 -17
============================================
+ Hits 100187 100270 +83
+ Misses 29351 29268 -83
- Partials 7962 7980 +18
```
| Flag | Coverage Δ | |
|---|---|---|
| inttests | `24.19% <0.93%> (-0.21%)` | :arrow_down: |
| systests | `24.87% <19.62%> (-0.15%)` | :arrow_down: |
| unittests | `72.21% <35.98%> (+0.08%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...a/org/apache/pulsar/broker/admin/v2/Functions.java](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9GdW5jdGlvbnMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...java/org/apache/pulsar/broker/admin/v2/Worker.java](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9Xb3JrZXIuamF2YQ==) | `11.76% <0.00%> (ø)` | |
| [...org/apache/pulsar/broker/admin/v2/WorkerStats.java](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9Xb3JrZXJTdGF0cy5qYXZh) | `40.00% <0.00%> (ø)` | |
| [...ons/worker/rest/api/v2/FunctionsApiV2Resource.java](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWZ1bmN0aW9ucy93b3JrZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9mdW5jdGlvbnMvd29ya2VyL3Jlc3QvYXBpL3YyL0Z1bmN0aW9uc0FwaVYyUmVzb3VyY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...s/worker/rest/api/v2/WorkerStatsApiV2Resource.java](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWZ1bmN0aW9ucy93b3JrZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9mdW5jdGlvbnMvd29ya2VyL3Jlc3QvYXBpL3YyL1dvcmtlclN0YXRzQXBpVjJSZXNvdXJjZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ulsar/functions/worker/rest/api/FunctionsImpl.java](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWZ1bmN0aW9ucy93b3JrZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9mdW5jdGlvbnMvd29ya2VyL3Jlc3QvYXBpL0Z1bmN0aW9uc0ltcGwuamF2YQ==) | `56.55% <16.66%> (-0.74%)` | :arrow_down: |
| [...org/apache/pulsar/broker/admin/impl/SinksBase.java](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL1NpbmtzQmFzZS5qYXZh) | `35.29% <35.71%> (ø)` | |
| [...g/apache/pulsar/broker/admin/impl/SourcesBase.java](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL1NvdXJjZXNCYXNlLmphdmE=) | `35.29% <35.71%> (-1.85%)` | :arrow_down: |
| [...ons/worker/rest/api/v3/FunctionsApiV3Resource.java](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWZ1bmN0aW9ucy93b3JrZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9mdW5jdGlvbnMvd29ya2VyL3Jlc3QvYXBpL3YzL0Z1bmN0aW9uc0FwaVYzUmVzb3VyY2UuamF2YQ==) | `43.39% <37.50%> (ø)` | |
| [...e/pulsar/functions/worker/rest/api/WorkerImpl.java](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWZ1bmN0aW9ucy93b3JrZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9mdW5jdGlvbnMvd29ya2VyL3Jlc3QvYXBpL1dvcmtlckltcGwuamF2YQ==) | `14.90% <38.46%> (+9.38%)` | :arrow_up: |
| ... and [16 more](https://codecov.io/gh/apache/pulsar/pull/20044?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [70 files with indirect coverage changes](https://codecov.io/gh/apache/pulsar/pull/20044/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Demogorgon314 commented on a diff in pull request #20044: [improve][client] Add retry when readTailMessages fails
Posted by "Demogorgon314 (via GitHub)" <gi...@apache.org>.
Demogorgon314 commented on code in PR #20044:
URL: https://github.com/apache/pulsar/pull/20044#discussion_r1161371766
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/TableViewImpl.java:
##########
@@ -266,18 +270,15 @@ private void readTailMessages(Reader<T> reader) {
handleMessage(msg);
readTailMessages(reader);
}).exceptionally(ex -> {
- logException(
- String.format("Reader %s was interrupted while reading tail messages.",
- reader.getTopic()), ex);
+ if (ex.getCause() instanceof PulsarClientException.AlreadyClosedException) {
+ log.error("Reader {} was closed while reading tail messages.",
+ reader.getTopic(), ex);
+ } else {
+ log.warn("Reader {} was interrupted while reading tail messages. "
Review Comment:
Do we need to delay few milliseconds before retry?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #20044: [improve][client] Add retry when readTailMessages fails
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #20044:
URL: https://github.com/apache/pulsar/pull/20044#issuecomment-1500970075
@lifepuzzlefun I updated the code to "retry-forever" upon `readTailMessages` failure. I assume the current "fail-stop" is not expected.
Another thing to note is that since the reader acks messages immediately, this "retry" will not retry the handling logic with the same message. (It will move on and retry the next message)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lifepuzzlefun commented on pull request #20044: [improve][client] Add isInterrupted func on TableView
Posted by "lifepuzzlefun (via GitHub)" <gi...@apache.org>.
lifepuzzlefun commented on PR #20044:
URL: https://github.com/apache/pulsar/pull/20044#issuecomment-1500809025
I understand the idea of this patch. but from a api user's view. it is confusing that what is an `interrupted`, and how should the api user react to the interrupted. I think should choose how to expose the internal exception on TableView to the api user. or we handle the exception inside the client with some retry method. : - )
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on a diff in pull request #20044: [improve][client] Add retry when readTailMessages fails
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #20044:
URL: https://github.com/apache/pulsar/pull/20044#discussion_r1161409510
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/TableViewImpl.java:
##########
@@ -266,18 +270,15 @@ private void readTailMessages(Reader<T> reader) {
handleMessage(msg);
readTailMessages(reader);
}).exceptionally(ex -> {
- logException(
- String.format("Reader %s was interrupted while reading tail messages.",
- reader.getTopic()), ex);
+ if (ex.getCause() instanceof PulsarClientException.AlreadyClosedException) {
+ log.error("Reader {} was closed while reading tail messages.",
+ reader.getTopic(), ex);
+ } else {
+ log.warn("Reader {} was interrupted while reading tail messages. "
Review Comment:
I think we can rely on consumer.receive logic for waiting. I don't think we need additional delay 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org