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

[history] Allow using :memory: db to avoid disk writes. #664

Closed

Conversation

dmach
Copy link

@dmach dmach commented Dec 29, 2018

New context methods to enable/disable history writes:
dnf_context_get_write_history()
dnf_context_set_write_history()

When set to false, history gets written into
an in-memory database rather than on disk.

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably 2b32f55) made this pull request unmergeable. Please resolve the merge conflicts.

@dmach dmach force-pushed the in-memory-history branch from d766aa9 to eb4c3bc Compare December 29, 2018 11:04
Copy link
Contributor

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Still need to test this patch in rpm-ostree, but yeah this definitely looks cleaner! 👍

@@ -229,6 +230,7 @@ dnf_context_init(DnfContext *context)
priv->check_transaction = TRUE;
priv->enable_filelists = TRUE;
priv->zchunk = TRUE;
priv->write_history = FALSE;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should default to TRUE, no? Otherwise, we're regressing everywhere other than rpm-ostree.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we should be defaulting to TRUE here.

Copy link
Author

Choose a reason for hiding this comment

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

ACK, changed to TRUE.

@jlebon
Copy link
Contributor

jlebon commented Jan 7, 2019

Yup, this is working fine here.

// check if DB file is present
if (!pathExists(path.c_str())) {
else if (!pathExists(path.c_str())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: this formatting looks odd to me. Let's just put it on the same line as the previous closing brace?

Copy link
Author

Choose a reason for hiding this comment

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

I tweaked the comments a little bit, it's hopefully better now.

@jlebon
Copy link
Contributor

jlebon commented Jan 15, 2019

☎️, can we get this in?

@jlebon
Copy link
Contributor

jlebon commented Feb 15, 2019

@dmach Mind fixing this up so we can get it in?

Copy link
Contributor

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Just minor tweaks otherwise LGTM!

* dnf_context_get_write_history
* @context: a #DnfContext instance.
*
* Gets whether writing to history database is enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor/optional: missing final period

Copy link
Author

Choose a reason for hiding this comment

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

fixed

*
* Returns: %TRUE if writing to history database is enabled
*
* Since: 0.25.0
Copy link
Contributor

Choose a reason for hiding this comment

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

This and below should be 0.27.0 now it seems?

Copy link
Member

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

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

One small thing, otherwise LGTM!

*
* Enables or disables writing to history database.
*
* Since: 0.25.0
Copy link
Member

Choose a reason for hiding this comment

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

Just bump this version to match what it should be...

Copy link
Author

Choose a reason for hiding this comment

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

fixed

New context methods to enable/disable history writes:
dnf_context_get_write_history()
dnf_context_set_write_history()

When set to false, history gets written into
an in-memory database rather than on disk.
@dmach dmach force-pushed the in-memory-history branch from d952bcd to 84c4f00 Compare February 26, 2019 14:46
@j-mracek j-mracek self-assigned this Feb 26, 2019
@j-mracek
Copy link
Contributor

LGTM

@j-mracek
Copy link
Contributor

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit 84c4f00 has been approved by j-mracek

@rh-atomic-bot
Copy link

⌛ Testing commit 84c4f00 with merge aca6c3f...

@rh-atomic-bot
Copy link

☀️ Test successful - status-papr
Approved by: j-mracek
Pushing aca6c3f to master...

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.

6 participants