Skip to content

Conversation

@vinaysingri
Copy link

No description provided.


public interface MessageConfigStore {

Optional<MessageConfig> create(MessageConfig messageConfig);

Choose a reason for hiding this comment

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

should create return optional ? if it fails, it should throw the exception right rather than fail internally ?

String messageId;

@NotNull
JsonNode messageBody;

Choose a reason for hiding this comment

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

lets mark them private

@AllArgsConstructor
@NoArgsConstructor
@Builder
public class StoredMessageConfig {

Choose a reason for hiding this comment

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

can we merge this with MessageConfig ?

public MessageConfigStoreCommand(LookupDao<StoredMessageConfig> messageLookupDao) {
this.messageLookupDao = messageLookupDao;
messageConfigCache = Caffeine.newBuilder()
.maximumSize(1_000)

Choose a reason for hiding this comment

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

what does 1_000 mean ?

Copy link
Author

Choose a reason for hiding this comment

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

Just a digit seperator notion that I took from other classes. For grouping digits for better readability

messageConfigCache = Caffeine.newBuilder()
.maximumSize(1_000)
.expireAfterWrite(300, TimeUnit.SECONDS)
.refreshAfterWrite(60, TimeUnit.SECONDS)

Choose a reason for hiding this comment

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

whats expire after write ? also refresh can be slower and 15 mins should be fine IMO

Copy link
Author

Choose a reason for hiding this comment

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

expire after write is basically looks at last write time and expires the entry after specified time, we can increase it and sure I will increase the refresh time

@Override
public Optional<MessageConfig> get(String messageConfigId) {
try {
return messageConfigCache.get(messageConfigId);

Choose a reason for hiding this comment

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

how are we pre populating data ? should we fall back to DB if cache miss occurs ? because ideally we don't expect to query for a msg id which doesn't exist. so either msg is present in cache or in DB.

or are we pre populating the DB somehow ? in that case how will new writes get cached ?

Copy link
Author

Choose a reason for hiding this comment

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

We have this method called getFromDb which is hooked into caffine while building the cache, so any cache miss will lead to a DB call.

@krishan1390
Copy link

lets also add comments for class responsibilites

throw new StatesmanError("No message config found", ResponseCode.NO_PROVIDER_FOUND);
}

MessageConfig storeOutput = optionalMessageConfig.get();

Choose a reason for hiding this comment

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

can we add a comment of the json structure ? will help whoever will read this code in future

Copy link
Author

Choose a reason for hiding this comment

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

done

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