Qa 7710 #25
Qa 7710 #25
Conversation
| } | ||
|
|
||
| func NewNode( | ||
| key string, |
There was a problem hiding this comment.
добавил комент в код
уникальный ключ, по которому мы понимаем как нам найти этот объект во внешнем мире + за то чтобы не добавить второй раз одно и то же
значение может зависеть от стратегии
для постоянных нод ip:port
для временных pod.name
| PodCreationTimeout time.Duration | ||
| } | ||
|
|
||
| func (sp *strategyParams) UnmarshalJSON(b []byte) error { |
There was a problem hiding this comment.
Выглядет так что тебе нужно добавить валидацию конфигов и тип type CfgDuration string с методом getStdlibDuration() time.Duration
There was a problem hiding this comment.
нужно координально все связанное с конфигами переделать, пока предлагаю оставить так как временное решение и вернуться к этому позже
| case <-stop: | ||
| return "", fmt.Errorf("wait podIP stopped by timeout, %v", podName) | ||
| default: | ||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
А что будет если человек по не знанию поставит p.podCreationTimeout = 500 * time.MIliseconds )))) наверно надо тоже в конфиг засунуть
There was a problem hiding this comment.
в конфиг засовывать не стоит, так как это его лишь усложнит его
сейчас пользователь просто повисит секунду + время на запрос в куб вместо 1/2 секунды, ничего страшного не пройзойдет
если мы хотим управлять частотой опроса можно делать это так req_f = podCreationTimeout / 3 если podCreationTimeout < 3 sec
но в этом тоже не много смысла, ибо время создания селениума в кубе всегда больше 1 секунды и если человек поставит маленький таймаут то он никогда не дождется результата
There was a problem hiding this comment.
Нет, он просто все время с таймаутом будет валиться, валидируй тогда значение на входе
There was a problem hiding this comment.
возникает вопрос какой таймаут считать минимальным и нужно ли это?
до первого issue будем верить в осознанность пользователей
| for { | ||
| select { | ||
| case <-stop: | ||
| return errors.New("wait stopped by timeout") | ||
| return "", fmt.Errorf("wait selenium stopped by timeout, %v", podName) | ||
| default: | ||
| time.Sleep(time.Second) |
pool/strategy/kubernetes/provider.go
Outdated
| err := p.clientset.CoreV1Client.Pods(p.namespace).Delete(podName, &apiV1.DeleteOptions{}) | ||
| if err != nil { | ||
| switch { |
There was a problem hiding this comment.
Что-то ты увлекся свичами, наверно вложенные условия логичнее.
There was a problem hiding this comment.
и то и то выглядит не очень 😩, но свич на мой вгляд проще читать
There was a problem hiding this comment.
if err != nil && !errHelper.HasNotFound(err) {
return errors.New("send command pod/delete to k8s, " + err.Error())
}
// or
if err != nil && !strings.Contains(err.Error(), "not found") {
return errors.New("send command pod/delete to k8s, " + err.Error())
}Выглядет сто пудов л��чше.
pool/strategy/kubernetes/strategy.go
Outdated
| if err != nil { | ||
| _ = s.provider.Destroy(podName) // на случай если что то успело создасться | ||
| go func(podName string) { | ||
| time.Sleep(time.Minute * 2) |
There was a problem hiding this comment.
Ух ты, я сейчас Эду напишу, он тебе там с Лехой айайай устроят)))
There was a problem hiding this comment.
хардкод в данном случае - это норма в конфиге он довольно бесполезен
There was a problem hiding this comment.
Я не говорю про конфиг, етсь константы с именем, что бы было ясно откуда 2 минуты и есть соотвественно вопрос, почему 2 минуты?
There was a problem hiding this comment.
вынес в константу
сделано так как есть предположение, что у нас может отвалиться ответ от апи но тем не менее под создастся
2 минуты взяты с потолка, я предполагаю что их хватит, в процессе эксплуатации поймем так это или нет
| } | ||
| if rowsAffected == 0 { | ||
| return storage.ErrNotFound | ||
| } |
There was a problem hiding this comment.
if rowsAffected > 1 {
return errors.New("have got two rows for one key")
}There was a problem hiding this comment.
это контроллируется на уровне индекса в бд
No description provided.