returning the error when doing a nack in the message

[cpanato]

Fixes: #831

other protocol return the error, doing similar thing here as well

[cpanato]

/assign @duglin

[duglin]

Seems reasonable. Is there a test we can add for this?

[duglin]

rebase is needed

[cpanato]

done @duglin, thanks for the review, PTAL

[duglin]

Is it possible to test that the error makes its way back to the logger?

[cpanato]

Is it possible to test that the error makes its way back to the logger?
did some manual test and saw that showing up in the logs.

but not sure how to do that here in the tests, do you have a tests that setup the pubsub? sorry for asking that, i am too new to this codebase

[duglin]

I looked around and I didn't see a good example of us testing that yet so perhaps I'm asking too much.
Unless @embano1 can think of an easy way to test it, this
/LGTM

[embano1]

Is it possible to test that the error makes its way back to the logger?

Are you asking for the caller here, i.e., integration tests?

[duglin]

Is it possible to test that the error makes its way back to the logger?

Are you asking for the caller here, i.e., integration tests?

Yea, I guess an integration test since the testcase added (while valid) is kind of silly/trivial since it's basically just testing one if-statement, and the real point of this PR was to ensure that the error bubbled up to the proper spot (like the logger). So, in essence not just test the if-statement but also the Finish() logic in conjunction with our logging system. But if we don't have an existing test that can be easily hooked-into I'm not sure it's fair to ask @cpanato to add one just for this trivial PR.

[cpanato]

maybe we can open an issue for that, I think that will require some more complex things to setup

[duglin]

maybe we can open an issue for that, I think that will require some more complex things to setup

Done: #979

I'm still ok with this PR. @embano1 for the final review/merge.

[embano1]

LGTM