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

Redis Publisher connection error in docker container #22

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

Hsankesara
Copy link
Member

Solved the issue #15. The error was because of dynamic import initialises default argument RedisConnection which leads it to connect with localhost:6379 and throw ConnectionError.

@yatharthranjan
Copy link
Member

I think tests are failing because of this - https://travis-ci.community/t/java-home-is-not-exported-for-arm64-architecture/6993

@Hsankesara
Copy link
Member Author

Yes, you're right.

Removing adoptopenjdk-11-hotspot (11.0.7+10-2) ...
E: Unable to locate package adoptopenjdk-8-hotspot

@Hsankesara
Copy link
Member Author

Should I edit the travis file to solve that?

@yatharthranjan
Copy link
Member

No, its ok to leave for now as travis should fix it soon. And we are planning to move to github actions soon.

@@ -11,7 +11,7 @@

class RedisPublisher(Publisher):

def __init__(self, connection: RedisConnection = RedisConnection(),
def __init__(self, connection: RedisConnection,
publisher_thread_pool: ThreadPoolExecutor = ThreadPoolExecutor(max_workers=4)):
Copy link
Member

Choose a reason for hiding this comment

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

Do you think the other defaults will also not be honoured? Like the thread_pool?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the issue here would only occur because of this default since it uses self.connect() init which would throw an exception when connection is not successful. Other default arguments do not share the same issue.

Copy link
Member

Choose a reason for hiding this comment

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

Ok great.

Copy link
Member

@yatharthranjan yatharthranjan left a comment

Choose a reason for hiding this comment

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

LGTM

@Hsankesara
Copy link
Member Author

Thanks!

@Hsankesara Hsankesara merged commit 3c1f926 into new-sensors Jan 20, 2021
@Hsankesara Hsankesara deleted the redis_url_fix branch January 20, 2021 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants