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

22329: Transactional support in direct client, MINOR #335

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dmaze
Copy link
Contributor

@dmaze dmaze commented Dec 7, 2024

Adds runtime options to the direct client. If you create a trainee as

Trainee(persistence='always', runtime={'transactional': True})

then every operation will append a log entry to the .caml file. This is faster per operation, and all of the data continues to be recorded on disk, but the resulting files will be larger than always-persist mode without the transactional option.

@dmaze dmaze force-pushed the 22329-transactional branch from 77fc50c to 5b9264b Compare December 9, 2024 15:30
howso/direct/client.py Outdated Show resolved Hide resolved
@@ -468,12 +485,14 @@ def _get_trainee_from_engine(self, trainee_id: str) -> Trainee:
persistence = metadata.get('persistence', 'allow')
trainee_meta = metadata.get('metadata')
trainee_name = metadata.get('name')
transactional = metadata.get('transactional', False)
Copy link
Member

@fulpm fulpm Dec 9, 2024

Choose a reason for hiding this comment

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

Seems like storing this in the model itself could easily get out of sync with the true transactional state. Is there way we could interrogate the entity to see its transactional state or track how its loaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the Amalgam C API layer, there's no way to query this back. The reverse of this is that there aren't that many ways to change the setting either: only the C LoadEntity, CloneEntity, and StoreEntity API calls can change it. The corresponding HowsoDirectClient.create_trainee(), copy_trainee(), and persist_trainee()calls all deal with the client-side metadata correctly. The one other place that modifies the metadata state isupdate_trainee()`, and that always preserves the current transactional setting.

I see the potential problem if something did manage to self.amlg.load_entity() without going through the client layer, but I'm not sure there's somewhere better to put the (per-process/per-Amalgam) value.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to store this in the python layer -- you wouldn't want to write this out because it is really a property of how you load it.

Adds runtime options to the direct client.  If you create a trainee as

    Trainee(persistence='always', runtime={'transactional': True})

then every operation will append a log entry to the .caml file.  This is
faster per operation, and all of the data continues to be recorded on
disk, but the resulting files will be larger than always-persist mode
without the transactional option.
@dmaze dmaze force-pushed the 22329-transactional branch from e6d5e98 to 00ca0e7 Compare December 18, 2024 18:14
Copy link
Contributor

github-actions bot commented Dec 18, 2024

Test Results

515 tests  +6   498 ✅ +5   59s ⏱️ -1s
  1 suites ±0    17 💤 +1 
  1 files   ±0     0 ❌ ±0 

Results for commit ccaf6c7. ± Comparison against base commit 51662ce.

♻️ This comment has been updated with latest results.

@dmaze dmaze marked this pull request as ready for review December 18, 2024 18:39
@dmaze dmaze requested a review from a team as a code owner December 18, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants