Bugfix/pn 288 fix sms loop retry#256
Conversation
|
@cosminrentea @bogh please take a look ASAP |
| gateway.route.Deliver(&msg, true) | ||
| // wait for retry to complete alongside with the 3 sent sms. | ||
| time.Sleep(500 * time.Millisecond) | ||
| a.Equal(uint64(4), gateway.LastIDSent, "Last id sent should be the msgID since the retry failed.Already try it for 4 time.Anymore retries would be useless") |
There was a problem hiding this comment.
nice documentation by test-code
| // deliver the message on the gateway route. | ||
| gateway.route.Deliver(&msg, true) | ||
| // wait for retry to complete alongside with the 3 sent sms. | ||
| time.Sleep(500 * time.Millisecond) |
There was a problem hiding this comment.
500 ms and 4 messages => are these values dependent on some constants in the code itself (like 100 ms, 3 retries)?
maybe: you could define&reuse some constants
| mockMessageStore.EXPECT().MaxMessageID(gomock.Any()).Return(uint64(2), nil) | ||
|
|
||
| //setup a new sms gateway | ||
| worker := 8 |
There was a problem hiding this comment.
what happens if other workers (not the one retrying to send) are sending correct messages at the same time?
is the lastMessageID correctly updated ?
There was a problem hiding this comment.
We have only one worker at one moment.Only the config value is set but not used afterwards
if *config.Workers <= 0 {
*config.Workers = connector.DefaultWorkers
}
So we can not have the situation you described.
As for workers maybe I should put a comment (something like will be used in future) or just delete it since we do not use it)
As for lastMessageID in the test we have 2 asserts which checks this value.
There was a problem hiding this comment.
didn't know that we have only one worker currently for sms - so how were the other correct sms sent the previous days?
There was a problem hiding this comment.
could you please add
- a TODO comment like you said
- a log on INFO showing the real "computed" number of workers for sms
There was a problem hiding this comment.
when the loop started I ussually saw that no other sms we're sent.but that was in the past.For this case I do not understand it either.
There was a problem hiding this comment.
for the LOG it is useless since we do not use config.Workers in any other places except tests.
I will add the TODO
| } | ||
|
|
||
| // when sms is sent simulate an error.No Sms will be delivered with success for 4 times. | ||
| mockSmsSender.EXPECT().Send(gomock.Eq(&msg)).Times(4).Return(ErrIncompleteSMSSent) |
There was a problem hiding this comment.
please, add a proof - maybe from Kibana - that this was the actual error that triggered the loop
There was a problem hiding this comment.
Say what? 😮
There was a problem hiding this comment.
As I was saying before the last deploy was done on Friday..so ...
There was a problem hiding this comment.
This loop error happened during Saturday and Sunday - are there any logs on error level ?
See here for a trace that shows the date+time of the deployments (including on Saturday):
https://logging-prod.rewe-digital.com/kibana4/#/discover?_g=(refreshInterval:(display:Off,pause:!f,section:0,value:0),time:(from:now-7d,mode:quick,to:now))&_a=(columns:!(wwIdent,sms_message,sms_scenario,sms_rule,sms_rule_case,module,msg,service),index:%5Btechnical-%5DYYYY.MM.DD,interval:auto,query:(query_string:(analyze_wildcard:!t,query:%27%22Registering%20ORM%20models%22%27)),sort:!(%27@timestamp%27,desc))
| } | ||
|
|
||
| // when sms is sent simulate an error.No Sms will be delivered with success for 4 times. | ||
| mockSmsSender.EXPECT().Send(gomock.Eq(&msg)).Times(4).Return(ErrIncompleteSMSSent) |
There was a problem hiding this comment.
This loop error happened during Saturday and Sunday - are there any logs on error level ?
See here for a trace that shows the date+time of the deployments (including on Saturday):
https://logging-prod.rewe-digital.com/kibana4/#/discover?_g=(refreshInterval:(display:Off,pause:!f,section:0,value:0),time:(from:now-7d,mode:quick,to:now))&_a=(columns:!(wwIdent,sms_message,sms_scenario,sms_rule,sms_rule_case,module,msg,service),index:%5Btechnical-%5DYYYY.MM.DD,interval:auto,query:(query_string:(analyze_wildcard:!t,query:%27%22Registering%20ORM%20models%22%27)),sort:!(%27@timestamp%27,desc))
| } | ||
|
|
||
| // when sms is sent simulate an error.No Sms will be delivered with success for 4 times. | ||
| mockSmsSender.EXPECT().Send(gomock.Eq(&msg)).Times(4).Return(ErrIncompleteSMSSent) |
3 participants
No description provided.