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

RavenDB-22294 Use Database.LastCompletedClusterTransactionIndex instead of `Database.RachisLogIndexNotifications.LastModifiedIndex #18407

Draft
wants to merge 2 commits into
base: v5.4
Choose a base branch
from

Conversation

haludi
Copy link
Contributor

@haludi haludi commented Apr 16, 2024

Issue link

https://issues.hibernatingrhinos.com/issue/RavenDB-22294

Additional description

The issue can happen if the client makes a "load document" request when the compare-exchange is already created but before the document is created.
This also required a cluster operation (like creating an index) to be executed in the middle

To fix that we will rely on the Database.LastCompletedClusterTransactionIndex that is updated only by the cluster transaction
instead of the Database.RachisLogIndexNotifications.LastModifiedIndex that can be updated by any cluster operation.

Type of change

  • Bug fix
  • Regression bug fix
  • Optimization
  • New feature

How risky is the change?

  • Low
  • Moderate
  • High
  • Not relevant

Backward compatibility

  • Non breaking change
  • Ensured. Please explain how has it been implemented?
  • Breaking change
  • Not relevant

Is it platform specific issue?

  • Yes. Please list the affected platforms.
  • No

Documentation update

  • This change requires a documentation update. Please mark the issue on YouTrack using Documentation Required tag.
  • No documentation update is needed

Testing by Contributor

  • Tests have been added that prove the fix is effective or that the feature works
  • Internal classes added to the test class (e.g. entity or index definition classes) have the lowest possible access modifier (preferable private)
  • It has been verified by manual testing

Testing by RavenDB QA team

  • This change requires a special QA testing due to possible performance or resources usage implications (CPU, memory, IO). Please mark the issue on YouTrack using QA Required tag.
  • No special testing by RavenDB QA team is needed

Is there any existing behavior change of other features due to this change?

  • Yes. Please list the affected features/subsystems and provide appropriate explanation
  • No

UI work

  • It requires further work in the Studio. Please mark the issue on YouTrack using Studio Required tag.
  • No UI work is needed

@haludi haludi requested a review from ayende April 16, 2024 15:56
@github-actions github-actions bot added the v5.4 label Apr 16, 2024
Copy link
Member

@ayende ayende left a comment

Choose a reason for hiding this comment

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

This looks okay, but also note this one: #18405

Where we touch on roughly the same area.

{
testObj = new TestObj{ Prop = $"new 2", Id = id };
await session2.StoreAsync(testObj);
await Assert.ThrowsAnyAsync<Exception>(async () => await session2.SaveChangesAsync());
Copy link
Contributor

Choose a reason for hiding this comment

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

let's assert the exact exception here

@haludi haludi changed the title Raven db 22202 RavenDB-22294 Use Database.LastCompletedClusterTransactionIndex instead of `Database.RachisLogIndexNotifications.LastModifiedIndex Apr 17, 2024
@arekpalinski
Copy link
Member

@haludi is it still draft PR?

@haludi
Copy link
Contributor Author

haludi commented Apr 24, 2024

@haludi is it still draft PR?

Yes, there are some topics we need to discuss and solve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants