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

Stop saving failures to flush jobs in InMemoryConfigStorage to disk #2336

Closed
wants to merge 1 commit into from

Conversation

gedimin45
Copy link
Contributor

I've found several problems with this approach:

  1. Call to \Google\Cloud\Core\Batch\HandleFailureTrait::initFailureFile leads to a RuntimeException when constructing the class.
  2. Persisting job flushing failures to disk is very unexpected from a class called with the words in memory in the name. This can lead to very unexpected situations, like the disk filling up.
  3. As far as I can tell, it is not useful unless google-cloud-batch is running. This is unexpected to users when the InMemoryConfigStorage is used - which is a default in the \Google\Cloud\Trace\TraceClient, for example.

What do you think about removing the failure handling features from InMemoryConfigStorage?

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 19, 2019
@dwsupplee
Copy link
Contributor

@Ged15, thanks for taking the time to organize this. I'll take some time shortly to fully review the proposal.

@gedimin45
Copy link
Contributor Author

This is not really going to help in my case since \Google\Cloud\Core\Batch\BatchJob::__construct calls \Google\Cloud\Core\Batch\HandleFailureTrait::initFailureFile as well.

My problem is that an exception is thrown when the batch failure dir cannot be created because it exists. To fix that, I've made a separate PR.

This PR can still be merged but the problem is that \Google\Cloud\Core\Batch\BatchJob::__construct also calls \Google\Cloud\Core\Batch\HandleFailureTrait::initFailureFile.

@dwsupplee
Copy link
Contributor

@Ged15, what if instead of disabling the file usage completely - we had an environment variable that would allow for disabling it? I totally agree with your points, but it would be a breaking change to remove this functionality at the moment.

@jdpedrie
Copy link
Contributor

jdpedrie commented Mar 4, 2020

@Ged15 have you had a chance to review this based on @dwsupplee's most recent comment?

@willemstuursma
Copy link

I'm also really interested in this change. We have this directory running full with files when we are unable to send logs to Stackdriver. Unfortunately, writing to disk cannot be disabled.

@saranshdhingra saranshdhingra added the gh:run-tests Runs the github actions test suite label Jul 19, 2022
@saranshdhingra saranshdhingra removed the gh:run-tests Runs the github actions test suite label Sep 12, 2022
@saranshdhingra
Copy link
Contributor

I think this is taken care in #4982
If you pass the env variable GOOGLE_CLOUD_BATCH_DAEMON_FAILURE_DIR with a string false, that should prevent logging of the failed items.

I am closing this PR since 4982 is already merged. Feel free to reopen this if more help is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants