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.

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

feat: restart connection changes #11

Merged
merged 4 commits into from
Feb 28, 2023

Conversation

andihaskel
Copy link
Contributor

@andihaskel andihaskel commented Feb 21, 2023

Description

Se implementa un wrapper donde se realiza un restart de la session si hay un fallo de tipo gocql: no hosts available in the pool y se vuelve a intentar ejecutar la query N cantidad de intentos

Task Context

What is the current behavior?

What is the new behavior?

Additional Context

Checklist

How was it tested?

  • unit
  • integration
  • other (specify)

Updated services

  • Does your code follow the project style guide?
  • Have you added tests that cover the changes and run them?

qb/qdelete/delete_exec.go Outdated Show resolved Hide resolved
qb/qselect/select_exec.go Outdated Show resolved Hide resolved
runner/runner.go Outdated Show resolved Hide resolved
@andihaskel andihaskel force-pushed the feat/AH/restart-connection-changes branch from 8358840 to 55942ee Compare February 21, 2023 18:19
qb/qcount/tests/count_exec_test.go Outdated Show resolved Hide resolved
qb/qcount/count.go Outdated Show resolved Hide resolved
qb/qcount/tests/count_test.go Outdated Show resolved Hide resolved
qb/qinsert/insert.go Outdated Show resolved Hide resolved
qb/qinsert/tests/insert_exec_test.go Outdated Show resolved Hide resolved
qb/qinsert/tests/insert_test.go Outdated Show resolved Hide resolved
qb/qupdate/tests/update_exec_test.go Outdated Show resolved Hide resolved
qb/qupdate/tests/update_test.go Outdated Show resolved Hide resolved
qb/qupdate/update.go Outdated Show resolved Hide resolved
@andihaskel andihaskel force-pushed the feat/AH/restart-connection-changes branch from 92c366e to 51796e1 Compare February 21, 2023 20:14
@andihaskel andihaskel force-pushed the feat/AH/restart-connection-changes branch 10 times, most recently from 31b57d5 to af80f55 Compare February 23, 2023 22:07
qb/qb.go Outdated Show resolved Hide resolved
qb/qcount/count_exec.go Show resolved Hide resolved
qb/runner/mocks/client.go Outdated Show resolved Hide resolved
qb/runner/runner.go Outdated Show resolved Hide resolved
Comment on lines +133 to +138
_ = r.client.Restart()

//TODO: handle error
Copy link
Contributor

Choose a reason for hiding this comment

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

Por que no manejamos el error?

Copy link
Contributor Author

@andihaskel andihaskel Feb 24, 2023

Choose a reason for hiding this comment

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

deberiamos mandarlo a sentry o algo. Esto va a intentar hacer un restart N cantidad de attempts, si falla despues de los intentos lo va a retornar en execFunc

Copy link
Contributor

Choose a reason for hiding this comment

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

Digo, no deberíamos omitir el error que devueve el r.client.Restart()

Copy link
Collaborator

Choose a reason for hiding this comment

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

se quito porque alli esta el dema del log de debug, tuvimos probelas de serializacion porque el cliente tiene la config que a si vez tiene una propiedad que no es serializable a json PrintFn y esto generaba un panic en el framework. la idea es que pongamos un sistema de logging mas estandarizado como lo tiene por ejemplo gorm que es una interface independiente modificable.

qb/runner/runner.go Show resolved Hide resolved
@andihaskel andihaskel force-pushed the feat/AH/restart-connection-changes branch 3 times, most recently from 7f628ca to 3828e3e Compare February 24, 2023 15:45
qb/client.go Outdated
return nil
}

func getSession(c models.Config) (*gocql.Session, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Podemos ponerle createSession?

getSession me suena mas a un singleton

qb/client.go Outdated
Comment on lines 141 to 131
func NewClientWithSession(session *gocql.Session, conf models.Config) (Client, error) {
c := &client{
session: session,
config: conf,
canRestart: false,
}

return c, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func NewClientWithSession(session *gocql.Session, conf models.Config) (Client, error) {
c := &client{
session: session,
config: conf,
canRestart: false,
}
return c, nil
}
func NewClientWithSession(session *gocql.Session, conf models.Config) Client {
return &client{
session: session,
config: conf,
canRestart: false,
}
}

@andihaskel andihaskel force-pushed the feat/AH/restart-connection-changes branch from 3828e3e to f39ce44 Compare February 27, 2023 20:17
qb/client.go Outdated
Comment on lines 139 to 131
func NewClientWithSession(session *gocql.Session, conf models.Config) (Client, error) {
return &client{
session: session,
config: conf,
canRestart: false,
}, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func NewClientWithSession(session *gocql.Session, conf models.Config) (Client, error) {
return &client{
session: session,
config: conf,
canRestart: false,
}, nil
}
func NewClientWithSession(session *gocql.Session, conf models.Config) Client {
return &client{
session: session,
config: conf,
canRestart: false,
}
}

Comment on lines 26 to 27
t.Errorf("exp: test got: %v", q.table)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Errorf("exp: test got: %v", q.table)
return
t.FatalF("exp: test got: %v", q.table)

Comment on lines 32 to 33
t.Errorf("exp: test2 got: %v", q.table)
return
Copy link
Contributor

@gabrielbursztein2 gabrielbursztein2 Feb 27, 2023

Choose a reason for hiding this comment

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

Suggested change
t.Errorf("exp: test2 got: %v", q.table)
return
t.FatalF("exp: test2 got: %v", q.table)

client := mocks.NewClient(t)

q := New(client).Fields(test.fields...).Into(test.table).Values(test.values...)
query := q.Build()
Copy link
Contributor

Choose a reason for hiding this comment

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

No te deja usarlo si es privado? Esta dentro del mismo paquete 🤔

@andihaskel andihaskel force-pushed the feat/AH/restart-connection-changes branch from f39ce44 to 7872a0a Compare February 27, 2023 22:31
@andihaskel andihaskel force-pushed the feat/AH/restart-connection-changes branch from 7872a0a to f572221 Compare February 27, 2023 22:36
Copy link
Contributor

@gabrielbursztein2 gabrielbursztein2 left a comment

Choose a reason for hiding this comment

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

🥇

@andihaskel andihaskel merged commit be0dd49 into main Feb 28, 2023
@andihaskel andihaskel deleted the feat/AH/restart-connection-changes branch February 28, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants