Skip to content

Conversation

SarrahElm
Copy link
Contributor

@SarrahElm SarrahElm commented Jul 18, 2025

Close #12432

Firstly, I adapt InfraCache to store the infra_version version of the current infrastructure.
Then, I adapt the apply_edit function (called by the edition endpoint) to store cache operations in valkey using zadd, as follows :

  • score -> the latest infra_version version
  • key -> infra_patches.{OSRD_VERSION}.{infra_id}
  • value -> List of cache operations

@github-actions github-actions bot added the area:editoast Work on Editoast Service label Jul 18, 2025
@SarrahElm SarrahElm changed the title Sei/editoast/adapt edition endpoint using zadd 12432 Adapt edition endpoint using zadd Jul 21, 2025
@SarrahElm SarrahElm changed the title Adapt edition endpoint using zadd editoast: adapt edition endpoint using zadd Jul 21, 2025
@SarrahElm SarrahElm force-pushed the sei/editoast/adapt-edition-endpoint-using-zadd-12432 branch 4 times, most recently from 92a5674 to da4c4ec Compare July 23, 2025 13:07
@SarrahElm SarrahElm requested a review from Khoyo July 25, 2025 07:44
@SarrahElm SarrahElm force-pushed the sei/editoast/adapt-edition-endpoint-using-zadd-12432 branch from da4c4ec to 17617b3 Compare July 25, 2025 13:55
@SarrahElm SarrahElm marked this pull request as ready for review July 25, 2025 13:56
@SarrahElm SarrahElm requested a review from a team as a code owner July 25, 2025 13:56
@SarrahElm SarrahElm requested review from leovalais and hamz2a July 25, 2025 13:59
Copy link
Member

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

LGTM. Therefore, I found a big issue in our conception.

@Khoyo While testing this PR I found out that we can not push duplicate values with different zindex.

Here an example where I've edited a detector position from 2820 -> 2821 -> 2820 -> 2819. We only have three values instead of four.

1) "[{\"Update\":{\"Detector\":{\"obj_id\":\"DB0\",\"track\":\"TB0\",\"position\":2820.0}}}]"
2) "[{\"Update\":{\"Detector\":{\"obj_id\":\"DB0\",\"track\":\"TB0\",\"position\":2821.0}}}]"
3) "[{\"Update\":{\"Detector\":{\"obj_id\":\"DB0\",\"track\":\"TB0\",\"position\":2819.0}}}]"

Copy link
Contributor

@hamz2a hamz2a left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@SarrahElm SarrahElm added this pull request to the merge queue Jul 28, 2025
@Khoyo Khoyo removed this pull request from the merge queue due to a manual request Jul 28, 2025
@Khoyo
Copy link
Contributor

Khoyo commented Jul 28, 2025

LGTM. Therefore, I found a big issue in our conception.

@Khoyo While testing this PR I found out that we can not push duplicate values with different zindex.

Here an example where I've edited a detector position from 2820 -> 2821 -> 2820 -> 2819. We only have three values instead of four.

1) "[{\"Update\":{\"Detector\":{\"obj_id\":\"DB0\",\"track\":\"TB0\",\"position\":2820.0}}}]"
2) "[{\"Update\":{\"Detector\":{\"obj_id\":\"DB0\",\"track\":\"TB0\",\"position\":2821.0}}}]"
3) "[{\"Update\":{\"Detector\":{\"obj_id\":\"DB0\",\"track\":\"TB0\",\"position\":2819.0}}}]"

Indeed. We could wrap those in a struct with a 64 bit nonce to avoid that behavior, wdyt?

@flomonster
Copy link
Member

LGTM. Therefore, I found a big issue in our conception.
@Khoyo While testing this PR I found out that we can not push duplicate values with different zindex.
Here an example where I've edited a detector position from 2820 -> 2821 -> 2820 -> 2819. We only have three values instead of four.

1) "[{\"Update\":{\"Detector\":{\"obj_id\":\"DB0\",\"track\":\"TB0\",\"position\":2820.0}}}]"
2) "[{\"Update\":{\"Detector\":{\"obj_id\":\"DB0\",\"track\":\"TB0\",\"position\":2821.0}}}]"
3) "[{\"Update\":{\"Detector\":{\"obj_id\":\"DB0\",\"track\":\"TB0\",\"position\":2819.0}}}]"

Indeed. We could wrap those in a struct with a 64 bit nonce to avoid that behavior, wdyt?

Sounds good to me.

@SarrahElm SarrahElm force-pushed the sei/editoast/adapt-edition-endpoint-using-zadd-12432 branch from 5bf08a5 to f314f94 Compare July 28, 2025 13:41
@SarrahElm SarrahElm force-pushed the sei/editoast/adapt-edition-endpoint-using-zadd-12432 branch from f314f94 to 7f22438 Compare July 28, 2025 13:42
@SarrahElm SarrahElm requested a review from flomonster July 29, 2025 07:46
Copy link
Member

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix 👍

@SarrahElm SarrahElm added this pull request to the merge queue Jul 29, 2025
Merged via the queue into dev with commit 2ece45f Jul 29, 2025
27 checks passed
@SarrahElm SarrahElm deleted the sei/editoast/adapt-edition-endpoint-using-zadd-12432 branch July 29, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:editoast Work on Editoast Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

editoast: adapt the edition endpoint to store cache operation in valkey using zadd operation
4 participants