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

ACK Queue: clear_acked_data() behavior #78

Closed
employee-1234567 opened this issue Nov 21, 2018 · 4 comments · Fixed by #127
Closed

ACK Queue: clear_acked_data() behavior #78

employee-1234567 opened this issue Nov 21, 2018 · 4 comments · Fixed by #127
Labels

Comments

@employee-1234567
Copy link

Playing around with the clear_acked_data() function, it seems to hang on to the last 1000 acked queue items. Why is that? I've already acked the data, yet disk space continued to be used.

Looking at the code in question:

    @sqlbase.with_conditional_transaction
    def clear_acked_data(self):
        sql = """DELETE FROM {table_name}
            WHERE {key_column} IN (
                SELECT _id FROM {table_name} WHERE status = ?
                ORDER BY {key_column} DESC
                LIMIT 1000 OFFSET {max_acked_length}
            )""".format(table_name=self._table_name,
                        key_column=self._key_column,
                        max_acked_length=self._MAX_ACKED_LENGTH)
        return sql, AckStatus.acked

It seems that self._MAX_ACKED_LENGTH is a private member constant. Can this not be made tunable by the user (e.g.. a kwarg in init for the class)?

I opened my resulting sqlite data files and manually ran:

DELETE FROM ack_queue_default WHERE status = 5;
VACUUM;

Which reduced the file size by several GB. Unless there is some edge case, surely you'd want to do something more like this?

    @sqlbase.with_conditional_transaction
    def clear_acked_data(self):
        sql = """DELETE FROM {table_name} WHERE status = ?""".format(table_name=self._table_name)
        return sql, AckStatus.acked

    @sqlbase.with_conditional_transaction
    def shrink_disk_usage(self):
        sql = """VACUUM"""
        return sql, None
@peter-wangxu
Copy link
Owner

@employee-1234567 do you mind providing a PR based on you mentioned above? I am pleased if you can do that.

Thanks
Peter

@employee-1234567
Copy link
Author

Sure, I'd be happy to. Before I make the PR, can you confirm if the propsed SQL is correct? I still feel like I'm missing something or I didn't understand the original intent. Is there a reason the existing code is keeping 1000 ACK'd items around?

@peter-wangxu
Copy link
Owner

@employee-1234567 please go ahead with your GH-82, the comment #82 (comment) had confirmed it's fine to proceed.

Thanks
Peter

@dcj
Copy link

dcj commented Mar 14, 2019

Doesn't seem like @employee-1234567 is going to revise the PR.
Nevertheless, a PR was submitted which is only stylistically incompatible.
Any chance this fix could revised/committed?

peter-wangxu added a commit that referenced this issue Mar 9, 2020
peter-wangxu added a commit that referenced this issue Mar 9, 2020
peter-wangxu added a commit that referenced this issue Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants