telefonicasc / etl-framework

Framework de ETLs (librerias, documentación, best practices, etc.)
GNU Affero General Public License v3.0
2 stars 5 forks source link

Nueva clase IoT #70

Closed CeciliaFili closed 7 months ago

CeciliaFili commented 7 months ago

Edit (fgalan): issue https://github.com/telefonicasc/etl-framework/issues/55

Se pueden encontrar más detalles de las funciones en los archivos best_practices.md y README.md

fgalan commented 7 months ago

Tras revisarlo en conjunto con @arcosa (autor principal de esta libreria) os pasamos el siguiente feedback:

Como comentario principal a la PR, sería conveniente homogeneizar a como se hace con otras clases y adoptar un patrón de controladores para los componentes de plataforma. Es decir, al igual que tenemos un cbManager para el componente Context Broker, que se instancia y luego se usan métodos con él (pe. send_batch, get_entities, etc.), sería conveniente hacer algo parecido para los IOTAs. En concreto, definir una iotaManager cuyo constructor use estos parámetros:

y los métodos send_json y send_batch ya no tendrían req_url y api_key.

Con respecto a time_sleep, también por homogeneidad haría como en el caso del CB: renombrarlo a sleep_send_batch con un valor por defecto (en el cb es 0, es decir no se espera entre envíos) y definido en el constructor para no repetirlo en las llamadas send_json y send_batch. Por otro lado, en entornos productivos es posible que se produzcan microcortes de conexión cuando se quieren enviar datos, sería aconsejable incorporar un mecanismo timeout/try similar al que se usa en el caso del CB, que tenemos un timeout (10 por defecto), post_retry_connect (3 por defecto), post_retry_backoff_factor (20 por defecto) definidos en el manager, que se pueden modificar a través del constructor y que send_batch (en este caso send_json) le da uso, en caso de tener timeouts hacer un número de retrys automáticamente, dará estabilidad en el envío de esos datos. Este mecanismo de timeout/try igual se puede dejar en deuda (en tal caso, no estaría dentro del scope de esta PR y abriríamos un issue sobre ello, decidnos por favor vuestro parecer).

El soporte de dataframes en el método de send_batch nos parece una buena idea a fin de aumentar la flexibilidad de uso de la librería. Hemos abierto un issue (https://github.com/telefonicasc/etl-framework/issues/71) para incorporar esta misma funcionalidad en el send_batch del CB (quedando fuera del scope de esta PR). En todo caso, en vez de tocar en etls/hello-world/requirements.txt, si se quiere indicar que pandas es un requisito de la librería, sería en el INSTALL_REQUIRES de python-lib/tc_etl_lib/setup.py

(A continuación habrá una sugerencia de renaming de algunas cosas, pero hasta aquí me refiero a los nombres antiguos, para no liar).

Luego, algunas consideraciones adicionales:

xavi12p commented 7 months ago

Hola, respecto a esto:

Por otro lado, en entornos productivos es posible que se produzcan microcortes de conexión cuando se quieren enviar datos, sería aconsejable incorporar un mecanismo timeout/try similar al que se usa en el caso del CB, que tenemos un timeout (10 por defecto), post_retry_connect (3 por defecto), post_retry_backoff_factor (20 por defecto) definidos en el manager, que se pueden modificar a través del constructor y que send_batch (en este caso send_json) le da uso, en caso de tener timeouts hacer un número de retrys automáticamente, dará estabilidad en el envío de esos datos. Este mecanismo de timeout/try igual se puede dejar en deuda (en tal caso, no estaría dentro del scope de esta PR y abriríamos un issue sobre ello, decidnos por favor vuestro parecer).

Estamos de acuerdo en incorporar ese mecanismo de timeout/try y vemos adecuado que se aborde posteriormente, como deuda técnica.

Corregimos el resto de temas que nos indicáis y actualizamos el PR.

fgalan commented 7 months ago

Hola, respecto a esto:

Por otro lado, en entornos productivos es posible que se produzcan microcortes de conexión cuando se quieren enviar datos, sería aconsejable incorporar un mecanismo timeout/try similar al que se usa en el caso del CB, que tenemos un timeout (10 por defecto), post_retry_connect (3 por defecto), post_retry_backoff_factor (20 por defecto) definidos en el manager, que se pueden modificar a través del constructor y que send_batch (en este caso send_json) le da uso, en caso de tener timeouts hacer un número de retrys automáticamente, dará estabilidad en el envío de esos datos. Este mecanismo de timeout/try igual se puede dejar en deuda (en tal caso, no estaría dentro del scope de esta PR y abriríamos un issue sobre ello, decidnos por favor vuestro parecer).

Estamos de acuerdo en incorporar ese mecanismo de timeout/try y vemos adecuado que se aborde posteriormente, como deuda técnica.

Perfecto. Creado issue al respecto https://github.com/telefonicasc/etl-framework/issues/72 y queda fuera del scope de la PR #70

mapedraza commented 5 months ago

Revisando la clase iotaManager, y sus métodos asociados, no es posible hacer un envío múltiple para distintos IDs. Esto sería útil para no tener que construir un objeto para cada uno de los IDs a enviar (si hay que actualizar múltiples dispositivos al final de una ETL).

Sería interesante la clase dispusiera de un método (i.e: send_http_multienty) en el cual se le pudiera pasar un diccionario con los ids de dispositivos como claves y el paquete de envío como valor.

En cualquier caso el apikey podría ser estático (normalmente no va a cambiar el apikey por ETL)