You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/10/10 20:00:24 UTC
[GitHub] [pulsar] flowchartsman opened a new pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
flowchartsman opened a new pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316
- This call will be made by gi.processResult if
- there is a result
- there were no system exceptions
- Fixes #12314
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.
**NOTES**
- due to the way the pulsar-function-go SDK is structured, adding regression tests to the Go code itself would be impossible without spinning up a complete environment; the call has been removed from the core loop of `startFunction()`, which depends on a complete environment with successfully-initialized client, producer, consumer, log producer and metrics server, as well as instance communication.
- This means that the best way to test this would be in the Java example tests or E2E testing, but I have been unable to locate any Java tests that exercise the metrics, so if there is a place I could add this easily, I would be most grateful (and even more so for a patch or PR :D )
This change added tests and can be verified as follows:
- for the reasons mentioned above, this test must be verified in situ for now
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): no
- The public API: no
- The schema: no
- The default values of configurations: no
- The wire protocol: no
- The rest endpoints: no
- The admin cli options: no
- Anything that affects deployment: no
-
### Documentation
Check the box below and label this PR (if you have committer privilege).
- [x] no-need-doc
As far as I can tell, this change restores the ProcessedSuccessfully metric to what it should be:
- <= ReceivedTotal
- Only incremented on a error-free function handler that produces a result.
--
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] flowchartsman edited a comment on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
flowchartsman edited a comment on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-939614019
I see. So it should be set no matter what, and, under normal operation processed_successfully and total_received will always be the same. This is personally disappointing because I was hoping to track input/output ratio with a counter and not being forced to use the user metric summaryvec is annoying, but I'll make the change to bring it in line with the Java version
--
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] freeznet edited a comment on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
freeznet edited a comment on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-1043813110
@flowchartsman would you please update this PR to the master branch? thanks.
--
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] flowchartsman commented on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-939628977
I have issued a new commit, which I believe clarifies the logic. The only part that does not entirely make sense to me is:
```go
if autoAck && !atMostOnce {
gi.ackInputMessage(msgInput)
}
gi.stats.incrTotalProcessedSuccessfully()
```
Why is it `!atMostOnce` inside the error-handling callback and
```go
if autoAck && atLeastOnce {
gi.ackInputMessage(msgInput)
}
```
if there is no message or output topic? Is this an error or is it to account for effectively-once somehow?
--
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] freeznet edited a comment on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
freeznet edited a comment on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-939617525
Yes, you are right, under normal conditions, `total_received` are the same as `processed_successfully`, and it proves the function runs without exception.
I like your idea of the output count metric, and I think we can impl that in a separate PR.
--
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] freeznet commented on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
freeznet commented on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-1048455219
@nlu90 please help to review this PR when you have time, thanks.
--
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] flowchartsman commented on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-939595258
Correct, but that is in the case where the function has no output topic,
and is already like that. So, should it be replaced? What is the behavior
of the other frameworks?
On Sun, Oct 10, 2021 at 9:04 PM Rui Fu ***@***.***> wrote:
> ***@***.**** commented on this pull request.
>
> Seems with this change, it will no longer having metrics recorded with
> this case:
> https://github.com/apache/pulsar/blame/master/pulsar-function-go/pf/instance.go#L376-L381
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/pulsar/pull/12316#pullrequestreview-775739289>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAHCJJF6TTKLVF3E3VPT55TUGIZY3ANCNFSM5FWZ7ZMA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
--
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] freeznet commented on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
freeznet commented on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-1043813110
@flowchartsman can you please update this PR to the master branch? thanks.
--
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] flowchartsman commented on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-1047895387
@freeznet done!
--
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] freeznet commented on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
freeznet commented on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-939617525
Yes, you are right, under normal conditions, `total_received` are the same as `processed_successfully `, and it proves the function runs without exception.
I like your idea of the output count metric, and I think we can impl that in a separate PR.
--
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] freeznet commented on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
freeznet commented on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-939603759
@flowchartsman this might be helpful https://github.com/apache/pulsar/blob/branch-2.9/pulsar-functions/instance/src/main/java/org/apache/pulsar/functions/instance/JavaInstanceRunnable.java#L342-L361
--
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] flowchartsman commented on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
flowchartsman commented on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-939614019
I see. So it should be set no matter what, and, under normal operation processed_successfully and total+received will always be the same. This is personally disappointing because I was hoping to track input/output ratio with a counter and not being forced to use the user metric summaryvec is annoying, but I'll make the change to bring it in line with the Java version
--
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] flowchartsman edited a comment on pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
flowchartsman edited a comment on pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316#issuecomment-939614019
I see. So it should be set no matter what, and, under normal operation processed_successfully and total_received will always be the same. This is personally disappointing because I was hoping to track input/output ratio with a counter and being forced to use the user metric summaryvec is annoying, but I'll make the change to bring it in line with the Java version
--
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] freeznet merged pull request #12316: [Issue 12314][Go Functions] Remove extra call to gi.stats.incrTotalProcessedSuccessfully()
Posted by GitBox <gi...@apache.org>.
freeznet merged pull request #12316:
URL: https://github.com/apache/pulsar/pull/12316
--
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