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

Update SDK to automatically pull runtime URL when a session starts #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bharatsuri97
Copy link
Contributor

No description provided.

return Optional.ofNullable(val);
}

public Optional<Object> getObject(String key) {

Choose a reason for hiding this comment

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

What would help here is to turn the Cache into a generic Cache<T>. That would let you avoid using the concrete type in BaseChatbotClientImpl and creating the set/getObject methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, we can make to use generic type.

this.basePath = basePath;
this.webClientBuilder = webClientBuilder;
this.clientWrapper = ClientFactory.createClient(basePath, webClientBuilder);
this.cache = new InMemoryCache(DEFAULT_TTL_SECONDS);

Choose a reason for hiding this comment

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

See note about making Cache generic here, then you can just have a protected Cache<Object> cache and handle the casting on a per-key basis (or create a POJO to store if you want to avoid casing between String|Object)

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we want to hardcode to InMemoryCache , it's better allow user to provide cache implementation.

return Optional.ofNullable(val);
}

public Optional<Object> getObject(String key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, we can make to use generic type.

@@ -25,7 +25,7 @@ public class RedisCache implements Cache {
private static final Long DEFAULT_TTL_SECONDS = 259140L; // 2 days, 23 hours, 59 minutes

private JedisPool jedisPool;
private long ttlSeconds;
private final long ttlSeconds;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also support storing/retrieving objects in RedisCache implementation.

this.basePath = basePath;
this.webClientBuilder = webClientBuilder;
this.clientWrapper = ClientFactory.createClient(basePath, webClientBuilder);
this.cache = new InMemoryCache(DEFAULT_TTL_SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we want to hardcode to InMemoryCache , it's better allow user to provide cache implementation.

@bharatsuri97 bharatsuri97 force-pushed the update-sdk-pull-runtime-url-dynamically branch from 5091e11 to 94017a6 Compare April 24, 2023 18:34
@bharatsuri97 bharatsuri97 force-pushed the update-sdk-pull-runtime-url-dynamically branch 2 times, most recently from 9f027e2 to 4ee9ac9 Compare May 2, 2023 23:12
@bharatsuri97 bharatsuri97 force-pushed the update-sdk-pull-runtime-url-dynamically branch from 4ee9ac9 to b8a9c37 Compare May 15, 2023 18:14
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.

3 participants