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

[CAMEL-16770] Added JDBC Idempotent Repository caching #5767

Merged
merged 6 commits into from
Jul 5, 2021

Conversation

mgenereu
Copy link
Contributor

For slow per-query SQL implementations like Snowflake, this new class caches the JDBC Idempotent Repository in one query. Just like the SQL Component's batch mode, this makes interactions with the server performant.

@mgenereu
Copy link
Contributor Author

mgenereu commented Jul 2, 2021

@davsclaus there is absolutely no rush on this. I just wanted to check that I did everything you needed from me on this PR and associated Jira ticket. I think it's right but if it's sitting because I forgot something, I'd like to address right away. Thank you!

Copy link
Contributor

@davsclaus davsclaus left a comment

Choose a reason for hiding this comment

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

Yeah looks good, just one question about the CAST function.

Also it would be good to add a section in the documentation (see src/main/docs) about this new cached repo so users better can know about it

private int hitCount;
private int missCount;
private String queryAllString
= "SELECT messageId, CAST(COUNT(*) AS INTEGER) AS messageCount FROM CAMEL_MESSAGEPROCESSED WHERE processorName = ? GROUP BY messageId";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is CAST a standard SQL function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, you brought up a good question for me around jdbcTemplate. The SQL statement clause for COUNT(*) returns a long on some SQL servers and an integer on others. While I was able to solve the problem on the SQL side for this low counts by casting (ANSI SQL-92 feature) to an integer, I feel like this should be done like the existing code in JdbcMessageIdRepository.java but I don't know how to cast two columns using jdbcTemplate. Are you or another Camel developer familiar with this so I can make this match the existing code style?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the code of the jdbc template it checks for what type you cast to Long vs Integer and does at rs.getInteger vs rs.getLong and then its up to the JDBC driver how to handle that.

If the number can be really really big, then we should maybe change that code to use Long instead of Integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally the number for an idempotent repository is 1 per processorName/messageId combination. To be honest, I'm not even sure why the count is even involved in the code. I just respected it for either past or future implementations. The documentation for the camel-sql component even says:

When working with concurrent consumers it is crucial to create a unique constraint on the columns processorName and messageId. Because the syntax for this constraint differs from database to database, we do not show it here.

This would result in the COUNT(*) always being 1.

The Long or Integer expectation comes from COUNT(*) and its relationship to the maximum number of rows for the underlying SQL/JDBC implementation. For Derby, it's an Integer and for Snowflake, it's a Long. For this use case of idempotency, there's no reasonable way it could be an actual long. I would keep the existing integer code.

I'll look into how to properly cast at the JDBC level and try to keep the SQL code a little more generic.

@mgenereu
Copy link
Contributor Author

mgenereu commented Jul 4, 2021

Also it would be good to add a section in the documentation (see src/main/docs) about this new cached repo so users better can know about it

Brief documentation added. Not really sure it needs more than "use this if you want the cache behavior". Would you expect more?

@mgenereu mgenereu marked this pull request as draft July 5, 2021 14:33
@mgenereu
Copy link
Contributor Author

mgenereu commented Jul 5, 2021

Okay. This seems closer to what we want. The jdbcTemplate now handles the casting in getInt and the code looks simpler and cleaner.

@mgenereu mgenereu marked this pull request as ready for review July 5, 2021 15:48
@mgenereu mgenereu requested a review from davsclaus July 5, 2021 15:48
@davsclaus davsclaus merged commit bf457b1 into apache:main Jul 5, 2021
@mgenereu mgenereu deleted the add-caching-jdbc-idempotent branch July 6, 2021 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants