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

Activejob mixin #1

Merged
merged 8 commits into from
Apr 15, 2016
Merged

Activejob mixin #1

merged 8 commits into from
Apr 15, 2016

Conversation

emilioeduardob
Copy link
Contributor

No description provided.

@ldlsegovia ldlsegovia merged commit a9f81c4 into master Apr 15, 2016
@emilioeduardob emilioeduardob deleted the activejob-mixin branch April 15, 2016 13:49
@blackjid
Copy link
Member

Me puse a sapear el PR y me llamaron la atencion los statuses....

Como que me falto diferencias el pending, entre working y queued
Me puse a invesigar y llegue a https://github.com/quirkey/resque-status y https://github.com/utgarda/sidekiq-status que son extensiones para resque y sidekiq para obtener y manjear el estado de un job....

Despues llegue a https://github.com/cdale77/active_job_status que hace lo mismo pero por sobre active job, pensaba que tal vez notify debiera trabajar por sobre esta otra, o no?? son dos cosas que son independientes. Tener la habilidad de preguntarle a un job por su estado y notificarlo a una ui de alguna manera.

eso 😁

@emilioeduardob
Copy link
Contributor Author

🐸 buena black..

@blackjid
Copy link
Member

que significa la ranita verde??? 😁

@emilioeduardob
Copy link
Contributor Author

es un sapo (sapeando)

@blackjid
Copy link
Member

jaja, tengo problemas parece..
pense que tenia que ver con algo sobre el comentario...

@ldlsegovia
Copy link
Member

@blackjid un par de cosas sobre active_job_status:

  • No hicimos diferencia entre working y queued porque en nuestra gema, no estamos mapeando directamente los estados del job. Los estados que manejamos están más relacionados al usuario final. En este sentido, working y queued para el usuario es simplemente pending. Aunque quizás sería bueno poder ver en que job se está trabajando en el momento.
  • Sobre si son independientes o no, creo que depende del enfoque. Para mi sin la parte de notificación, la gema se queda a la mitad. Teniendo una única gema, resolvemos el circuito completo de manera más transparente. Por ej: estamos proporcionando el helper job_identifier_for que permite generar el hash que el polling necesita para traer datos de una entidad (usuario) específica. Separando las gemas, el usuario tendría que hacerse cargo de esa parte.
  • Sobre la gema que pasaste, ya estabamos avanzados con lo que habíamos hecho pero me gustaría haber probado el tema de hacerlo con cache en vez de en db, como hacemos nosotros.
    Lo que no me gustó, es que:
    • Se necesita especificar store según el adapter y heredar de ActiveJobStatus::TrackableJob. Nosotros nos esforzamos en hacer que fuera totalmente transparente al adapter. Por lo que, el usar nuestra gema, sólo requiere cambiar perform por perform_with_feedback en el job. Si uno no quiere la funcionalidad de feedback, sólo debe llamar a perform como se acostumbra.
    • Cuando se crea un batch, le pasa un key así: my_key = "230923asdlkj230923". Con el mixin JobNotifier::Identifier, damos un camino para generar esos keys de manera más transparente.
    • No guarda el feedback de los jobs. Sólo guarda referencia a jobs en los batches. Se me ocurre que para agregar esa funcionalidad, terminaríamos haciendo sobre esa gema algo parecido a lo que hacemos en la nuestra.

@blackjid
Copy link
Member

Buena, pensaba que era muy adhoc para que notifier usara active_job_status por detras... y no tener que inventar la rueda denuevo, pero veo que hay diferencias y que ya estaba reinventada (con sus diferencias)

No hicimos diferencia entre working y queued porque...

Me parece... puede ser un enhancement para despues igual.. si es necesario

me gustaría haber probado el tema de hacerlo con cache en vez de en db

Si creo tambien que seria bueno eventualmente revisar eso... tal vez en la db tiene sentido hacerlo si estas usando delayed_jobs con db como store, pero tambien podrias usar redis. O si usas sidekiq o resque como en ninja-market se me hace mas sucio todavia tener una dependencia a la DB

Se necesita especificar store según el adapter y heredar de ActiveJobStatus::TrackableJob

Si efectivamente queda super limpio con el perform_with_feedback

No guarda el feedback de los jobs.

el feedback de los jobs es lo que le pasas en el block??

@ldlsegovia
Copy link
Member

Si creo tambien que seria bueno eventualmente revisar eso... tal vez en la db tiene sentido hacerlo si estas usando delayed_jobs con db como store, pero tambien podrias usar redis. O si usas sidekiq o resque como en ninja-market se me hace mas sucio todavia tener una dependencia a la DB

Una cosa es que adapter estoy usando para la app y otra que estoy usando para los jobs, no?
Quiero decir, en la gema estamos creando JobNotifier::Job que es un modelo de ActiveRecord pero, para la parte concerniente a los jobs de ActiveJob, no hemos restringido de ninguna manera los adapters que se pueden utilizar. Vos a qué te referís exactamente? Quizás me estoy perdiendo algo importante...

el feedback de los jobs es lo que le pasas en el block??

El feedback viene de lo que se ejecuta dentro de: perform_with_feedback. En el ejemplo del README, viene a ser el resultado de: MyService.run(param1, param2). Ese resultado, se guarda en una instancia de JobNotifier::Job (que está asociado al job "real" a través de job_id) en el atributo result.

@blackjid
Copy link
Member

Probablemente yo peco de 🐸 y hablo muy de guata mas que entendiendo a cabalidad.

Pensaria que el estado de los jobs es algo relativamente emifero... de ahi la idea de usar un cache con un expire. Por eso me parece mucho usar la db sobre todo si ni siquiera se esta usando en el adapted de active_job. Me parecerecia razonable guardar ese estado en el mismo store que use el adapter de active job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants