Skip to content

[fix][fn] Revert change to deprecation since it broke the master branch#19904

Merged
lhotari merged 1 commit intoapache:masterfrom
lhotari:lh-fix-pulsar-function-go-build
Mar 27, 2023
Merged

[fix][fn] Revert change to deprecation since it broke the master branch#19904
lhotari merged 1 commit intoapache:masterfrom
lhotari:lh-fix-pulsar-function-go-build

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Mar 23, 2023

Motivation

Master branch build fails with this error message:

Error: pf/instance.go:158:15: SA1019: gi.context.instanceConf.funcDetails.AutoAck is deprecated: Do not use.  (staticcheck)
			autoAck := gi.context.instanceConf.funcDetails.AutoAck
			           ^
Error: pf/instance.go:364:[13](https://github.com/apache/pulsar/actions/runs/4497924735/jobs/7913997105?pr=19903#step:7:14): SA1019: gi.context.instanceConf.funcDetails.AutoAck is deprecated: Do not use.  (staticcheck)
	autoAck := gi.context.instanceConf.funcDetails.AutoAck
	           ^
Error: pf/instanceConf.go:105:6: SA1019: instanceConf.funcDetails.AutoAck is deprecated: Do not use.  (staticcheck)
	if !instanceConf.funcDetails.AutoAck &&
	    ^
Error: Process completed with exit code 1.

Modifications

Revert change to deprecation

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

- change was made in apache#19470
- master branch build fails with "SA1019: instanceConf.funcDetails.AutoAck is deprecated: Do not use."
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 23, 2023
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

let's fix master branch

@lhotari lhotari closed this Mar 24, 2023
@lhotari lhotari reopened this Mar 24, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #19904 (ff7c4bb) into master (a903733) will decrease coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19904      +/-   ##
============================================
- Coverage     30.82%   30.61%   -0.21%     
+ Complexity      295      294       -1     
============================================
  Files          1680     1680              
  Lines        127262   127294      +32     
  Branches      13877    13879       +2     
============================================
- Hits          39230    38973     -257     
- Misses        82164    82484     +320     
+ Partials       5868     5837      -31     
Flag Coverage Δ
inttests 24.62% <ø> (+0.15%) ⬆️
systests 25.23% <ø> (-0.32%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 64 files with indirect coverage changes

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM.

Although, it seems a dangling follow-up to #15560.

cc @shibd how do we handle all these config options in prod and how we finally deprecate them? It seems to me that function go does not use ProcessingGuarantees.MANUAL at all.

@shibd
Copy link
Member

shibd commented Mar 27, 2023

how do we handle all these config options in prod and how we finally deprecate them?

Currently, you can always set autoAck to true, when you want to set autoAck = false, you should use Guarantees = MANUAL configuration instead.

This feature is supported in 2.11.0, and after the next 2~3 versions, we can delete the autoAck configuration.

It seems to me that function go does not use ProcessingGuarantees.MANUAL at all.

Actually used, In go, the MANUAL configuration is not used directly to make any judgments, but it is useful to replace the autoAck = false case.

For example, when autoAck = false or ProcessingGuarantees = MANUAL will not be ack.

// Otherwise the message succeeded. If the SDK is entrusted with responding and we are using
// atLeastOnce delivery semantics, ack the message.
if autoAck && atLeastOnce {
gi.ackInputMessage(msgInput)
}

@lhotari lhotari merged commit cda2827 into apache:master Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants