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.

Sign up for GitHub

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

Jump to bottom

FR: QOS Ack Implementation #93

Merged
merged 5 commits into from
Jul 19, 2022
Merged

FR: QOS Ack Implementation #93

merged 5 commits into from
Jul 19, 2022

Conversation

denopink
Copy link
Contributor

@denopink denopink commented Jul 8, 2022 •
edited
Loading

Related to xmidt-org/talaria#228 and xmidt-org/talaria#235
What this includes

  • make SupportsTransaction easier to read
  • add SupportsQOSAck
  • add IsQOSAckPart
  • add usefull default qos values
const (
	QOSLowValue QOSValue = iota * 25
	QOSMediumValue
	QOSHighValue
	QOSCriticalValue
)
  • add TestMessageTypeSupportsQOSAck
  • improve readability for SupportsTransaction and add missing message type case lastMessageType
  • update error message

@denopink
Add SupportsQOSAck func, improve SupportsTransaction func, update error
00829d4 
- make `SupportsTransaction` easier to read
- add missing message type for `TestMessageTypeSupportsTransaction`
- add `SupportsQOSAck`
- add `TestMessageTypeSupportsQOSAck`
- improve readability for `SupportsTransaction` and add missing message type case `lastMessageType`
- update error message
@denopink denopink added the enhancement New feature or request label Jul 8, 2022
@denopink denopink self-assigned this Jul 8, 2022
@denopink denopink marked this pull request as draft July 8, 2022 15:34
@codecov
Copy link

codecov bot commented Jul 8, 2022 •
edited
Loading

Codecov Report

Merging #93 (a347b65) into main (abcb3f7) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #93      +/-   ##
==========================================
+ Coverage   51.02%   51.06%   +0.03%     
==========================================
  Files          24       24              
  Lines        3953     3956       +3     
==========================================
+ Hits         2017     2020       +3     
  Misses       1760     1760              
  Partials      176      176
Flag Coverage Δ
unittests 51.06% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
qos.go 100.00% <ø> (ø)
messages.go 96.99% <100.00%> (+0.21%) ⬆️
messagetype.go 95.12% <100.00%> (-0.63%) ⬇️

@denopink denopink marked this pull request as ready for review July 19, 2022 19:03
denopink
denopink commented Jul 19, 2022
View reviewed changes
messages.go
@@ -201,6 +201,22 @@ func (msg *Message) TransactionKey() string {
return msg.TransactionUUID
}

// IsQOSAckPart determines whether or not a message can QOS ack.
func (msg *Message) IsQOSAckPart() bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to provide a quick way to check whether msgs are able to qos ack.

denopink
denopink commented Jul 19, 2022
View reviewed changes
messagetype.go
@@ -49,22 +49,22 @@ const (
// where applicable).
func (mt MessageType) SupportsTransaction() bool {
switch mt {
case Invalid0MessageType:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was looking to see how we did similar things as SupportsTransaction but then I noticed we can simplify it and improve readability

denopink
denopink commented Jul 19, 2022
View reviewed changes
qos.go
@@ -16,6 +16,13 @@ const (
// type determine what QOSLevel a message has.
type QOSValue int

const (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found myself wanting a quick default values for the talaria qos implementation and I can imagine other users will too.

@denopink denopink mentioned this pull request Jul 19, 2022
Merged
johnabass
johnabass approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@johnabass johnabass left a comment

Choose a reason for hiding this comment

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

good first approach! there's some things we probably want to evolve over the next few months, but this is a solid 1.0 version.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@denopink denopink merged commit f71e848 into main Jul 19, 2022
@denopink denopink deleted the denopink/feature/qos-ack-implementation branch July 29, 2022 19:04
@denopink denopink mentioned this pull request Aug 15, 2022
Merged
@denopink denopink mentioned this pull request Aug 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnabass johnabass johnabass approved these changes

@mtrinh11 mtrinh11 Awaiting requested review from mtrinh11

@renaz6 renaz6 Awaiting requested review from renaz6

Assignees

@denopink denopink

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@denopink @johnabass