feat: BLOCK_STRUCTURES_SETTINGS['PRUNING_ACTIVE'] toggle removed depr31 #32496
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @Yagnesh1998! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
mphilbrick211
added
the
needs test run
|
@Yagnesh1998 can you look at the test failures? |
e0d
removed
the
needs test run
| @@ -80,7 +79,6 @@ def _verify_update_or_create_call(self, serialized_data, mock_log=None, expect_c | |||
| return bsm | |||
|
|
|||
| @patch('openedx.core.djangoapps.content.block_structure.models.log') | |||
| @patch.dict(settings.BLOCK_STRUCTURES_SETTINGS, {'PRUNING_ACTIVE': False}) | |||
There was a problem hiding this comment.
@Yagnesh1998 since we now always run the pruner, we need to update this test further. It calls the _verify_update_or_create_call function above and assumes that the last log message will be about the creation or update but when the pruner is active, the last log message is about pruning rather than about whether the BlockStructure was created or updated.
If you run this test locally, you can put a breakpoint after line 74 in this file to see what's happening. Take a look at the output of mock_log.info.call_args_list in the debugger to get a better sense of what's going on. I think you'll have to update that helper function(_verify_update_or_create_call) to look at the last call to mock_log if it's a creation but look at the 2nd to last call to mock_log if it's an update.
remove white space.
|
@feanil all tests passed successfully. |
| if num_to_keep is None: | ||
| num_to_keep = config.num_versions_to_keep() | ||
|
|
||
| try: | ||
| all_files_by_date = sorted(cls._get_all_files(data_usage_key)) | ||
| files_to_delete = all_files_by_date[:-num_to_keep] if num_to_keep > 0 else all_files_by_date # lint-amnesty, pylint: disable=invalid-unary-operand-type | ||
| cls._delete_files(files_to_delete) | ||
| log.info( |
There was a problem hiding this comment.
@Yagnesh1998 I'm not sure that we want to delete this log message, although maybe it's worth making it a DEBUG message instead. @ormsbee is this message still useful as an INFO message from your perspective?
| if not created: | ||
| cls._prune_files(data_usage_key) | ||
|
|
||
|
|
||
| cls._log(bs_model, operation, serialized_data) |
There was a problem hiding this comment.
@feanil as per your suggestion add again log.info and changes here.
This issue occurs due to the logs. In the case of update, the prune_files method is called that delete the log file and update deleted logs in the log file so I call cls._logs after the prune_files method.
There was a problem hiding this comment.
@Yagnesh1998 I'm worried about moving the log message lower. It would mean if there is a failure in pruning, we will not get the log message about the document creation or update. That is confusing and might make it harder to debug the issue. However, looking at the atomic transaction, it looks like there is plenty of logging in the error handler there so I think this is fine to do and won't harm anything.
| if not created: | ||
| cls._prune_files(data_usage_key) | ||
|
|
||
|
|
||
| cls._log(bs_model, operation, serialized_data) |
There was a problem hiding this comment.
@Yagnesh1998 I'm worried about moving the log message lower. It would mean if there is a failure in pruning, we will not get the log message about the document creation or update. That is confusing and might make it harder to debug the issue. However, looking at the atomic transaction, it looks like there is plenty of logging in the error handler there so I think this is fine to do and won't harm anything.
|
Looks like this is ready to merge, I'll coordinate merging this. |
|
@Yagnesh1998 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Description:
As Per this DEPR: openedx/public-engineering#31
Removing: Toggle labeled 'BLOCK_STRUCTURES_SETTINGS['PRUNING_ACTIVE']. The toggle has the following annotation as a description:
When True, only a specified number of versions of block structure files are kept for each structure, and the rest are cleaned up. The number of versions that are kept can be specified in the BlockStructureConfiguration, which can be edited in Django Admin. The default number of versions that are kept is 5.
Supporting information:
as per the original jeera issue https://openedx.atlassian.net/browse/DEPR-146, this toggle is removed