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

[master < T517] On delete triggers invalid edge reference #717

Merged

Conversation

as51340
Copy link
Contributor

@as51340 as51340 commented Dec 15, 2022

[master < Epic] PR

  • Check, and update documentation if necessary
  • Update changelog
  • Write E2E tests
  • Compare the benchmarking results between the master branch and the Epic branch
  • Provide the full content or a guide for the final git message

[master < Task] PR

  • Check, and update documentation if necessary
  • Update changelog
  • Provide the full content or a guide for the final git message

@as51340 as51340 marked this pull request as ready for review December 15, 2022 14:05
@as51340 as51340 added the bug bug label Dec 15, 2022
@as51340 as51340 self-assigned this Dec 15, 2022
@gitbuda gitbuda added the v2.5.1 label Dec 15, 2022
@as51340 as51340 requested a review from jbajic December 16, 2022 22:36
Copy link
Contributor

@antaljanosbenjamin antaljanosbenjamin left a comment

Choose a reason for hiding this comment

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

I haven't checked the tests, because

tests/e2e/triggers/workloads.yaml Outdated Show resolved Hide resolved
tests/e2e/triggers/workloads.yaml Outdated Show resolved Hide resolved
tests/e2e/triggers/workloads.yaml Outdated Show resolved Hide resolved
tests/e2e/triggers/triggers_properties_false.py Outdated Show resolved Hide resolved
tests/e2e/triggers/triggers_properties_false.py Outdated Show resolved Hide resolved
@as51340
Copy link
Contributor Author

as51340 commented Dec 20, 2022

Two more questions from my side:

  1. Is this the correct logic if (delta->vertex_edge.edge != edge_) return !deleted;
  2. Is test_create_on_delete_explicit_transaction valid for all three isolation levels? Workflow:
  • create trigger
  • create and delete edge in the explicit transaction

src/storage/v2/edge_accessor.cpp Outdated Show resolved Hide resolved
tests/e2e/triggers/triggers_properties_false.py Outdated Show resolved Hide resolved
tests/e2e/triggers/common.py Outdated Show resolved Hide resolved
Copy link
Contributor

@antaljanosbenjamin antaljanosbenjamin left a comment

Choose a reason for hiding this comment

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

To answer your questions:

Is this the correct logic if (delta->vertex_edge.edge != edge_) return !deleted;

No, but as I see you already fixed it. Accessing the fields of a delta without knowing what type of delta is that is bad (in most of the cases you can end up with undefined behavior).

Is test_create_on_delete_explicit_transaction valid for all three isolation levels?

I would say it should be okay. The isolation levels are important when you work with multiple transaction simultaneously. In the test you don't have multiple transactions, so I would say the isolation levels should have no affect on the test. However there is always a possibility that I missed something or just plain wrong, so if you noticed something strange, feel free to bring it up, it might lead to interesting conversation on top of helping you.

@gitbuda
Copy link
Member

gitbuda commented Jan 17, 2023

There are still some comments left here. Can you resolve them all + uncomment the e2e test config?

@antaljanosbenjamin antaljanosbenjamin merged commit 156e2cd into master Jan 18, 2023
@antaljanosbenjamin antaljanosbenjamin deleted the T517-FL-invalid-edge-accessor-edge-reference branch January 18, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants