Skip to content

Use ClickHouseAutoIncrement rather than dao.getLargestId() - #168

Open
sheridancbio wants to merge 3 commits into
fix-delete-race-conditionfrom
eliminate-clinical-event-id-pregen
Open

Use ClickHouseAutoIncrement rather than dao.getLargestId()#168
sheridancbio wants to merge 3 commits into
fix-delete-race-conditionfrom
eliminate-clinical-event-id-pregen

Conversation

@sheridancbio

Copy link
Copy Markdown
Contributor

Several import classes were using their own business logic to increment internal ids based on the highest available integer value in the current database table.

This PR converts that business logic over to using the ClickHouseAutoIncrement class, and will also relocate the assignment of internal identifiers into the dao layer rather than the scripts layer.

@sheridancbio
sheridancbio requested review from forus and jamesqo July 31, 2026 18:06
@sheridancbio sheridancbio added the enhancement New feature or request label Jul 31, 2026
@sheridancbio
sheridancbio changed the base branch from main to fix-delete-race-condition July 31, 2026 18:07

@forus forus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

@cbioportalpipelines
cbioportalpipelines force-pushed the fix-delete-race-condition branch 2 times, most recently from 9fa9cf6 to c261e58 Compare August 7, 2026 16:33
- process per phase (create/populate/delete/drop) rather than per table
- use separate staging table for every table/idColumn pair
- wait for earlier phase to show completion before proceeding
- adjust java type of internal identifiers to Long (for clinical_event_ids)
- use system.parts table as an accurate records count
    - for confirming population of staging table
    - for confirming the consistency across cluster replicas (clickhouse.cloud)
@cbioportalpipelines
cbioportalpipelines force-pushed the eliminate-clinical-event-id-pregen branch from 11b281d to 896f3d7 Compare August 12, 2026 22:45

@jamesqo jamesqo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just one comment I have, then you should be good to merge.

Also, I think the fix-delete-race-condition has been merged into master. So if you wanted to rebase off of master before merging that might be a good idea.

private static final String DELETE_MUTATION = "DELETE from mutation WHERE genetic_profile_id=? and sample_id=?";

public static int addMutation(ExtendedMutation mutation, boolean newMutationEvent) throws DaoException {
public static void addMutation(ExtendedMutation mutation) throws DaoException {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see ClickHouseAutoIncrement called in this method? And don't think that insertRecord increments the database ID, either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants