Skip to content

Adding attempt events packaging batch job.#7259

Merged
Rwolfe-Nava merged 10 commits intomainfrom
lg7470-attempts-api-batch-job
Nov 8, 2022
Merged

Adding attempt events packaging batch job.#7259
Rwolfe-Nava merged 10 commits intomainfrom
lg7470-attempts-api-batch-job

Conversation

@Rwolfe-Nava
Copy link
Contributor

changelog: Internal, Attempts API, Add batch job to package events.

🎫 Ticket

Link to the relevant ticket.

🛠 Summary of changes

Added a batch job to read attempt events from the previous hour from redis. Writes those events to a file and encrypts the file. File is then stored in a placeholder location until we get an S3 bucket.

Want to get some early eyes on this. WIP and still needs lots of cleanup.

@Rwolfe-Nava Rwolfe-Nava force-pushed the lg7470-attempts-api-batch-job branch from 805629b to 5908209 Compare November 3, 2022 19:20
@Rwolfe-Nava Rwolfe-Nava changed the title WIP - Adding attempt events packaging batch job. Adding attempt events packaging batch job. Nov 3, 2022
@Rwolfe-Nava Rwolfe-Nava marked this pull request as ready for review November 3, 2022 19:44
Copy link
Contributor

Choose a reason for hiding this comment

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

another idea: let's make this write to the /tmp directory?

We can use ruby's Tempfile class to help make a file in there

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggested that we wanted to avoid Tempfile because it's eager to delete the file we just wrote. This will probably work fine once we're writing to S3, but for now it was just an annoyance and I figured we should treat what we're writing as if it persists.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggested that we wanted to avoid Tempfile because it's eager to delete the file we just wrote. This will probably work fine once we're writing to S3, but for now it was just an annoyance and I figured we should treat what we're writing as if it persists.

Comment on lines 220 to 225
Copy link
Contributor

Choose a reason for hiding this comment

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

this will get run in all environments regardless of if attempts api is enabled or not, which seems not good. let's either:

  1. add a check inside the job to bail if the api is not enabled
  2. only add the job to the schedule if the api enabled

and I know we have a lot of configs, but it seems like we may want to have a separate feature flag for just the attempts API background job so that we can smoothly transition to it if/when we are ready

Copy link
Contributor Author

@Rwolfe-Nava Rwolfe-Nava Nov 7, 2022

Choose a reason for hiding this comment

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

I think the first line inside the job checks if IRS attempts API is enabled and returns nil if it isn't, so it should only continue if it is enabled.

return nil unless IdentityConfig.store.irs_attempt_api_enabled

With that said, I do like the idea of ALSO not scheduling the job unless it is enabled, since I don't think that makes much sense. Plus, even though its probably not a huge resource tax, it's one less job the scheduler has to deal with if it doesn't need to.

Are there any examples of other jobs implementing similar functionality I can look at?

@Rwolfe-Nava Rwolfe-Nava force-pushed the lg7470-attempts-api-batch-job branch from 34a2f6b to 7b79225 Compare November 4, 2022 21:07
Comment on lines 16 to 19
Copy link
Contributor

Choose a reason for hiding this comment

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

in hindsight, this mkdir_p was masking the bug with the tmpdir being cleaned up beforehand, maybe we should remove it. that prevents the job from writing files where it's not supposed to

Suggested change
# write to a file and store on the disk until S3 is setup
FileUtils.mkdir_p(dir_path)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we actually need to get rid of this? I can see an argument for doing so, that whatever calls this shouldn't pass in a directory that doesn't exist. Is there some other way we can ensure safety / error handling if we remove this line?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this code will evolve over time. Eventually we'll write a temp file and upload that to S3. So in that case, the code could make it's own tmpdir and clean that up.

However in the meantime, this code hid a bug so I'd vote to remove it.

@Rwolfe-Nava Rwolfe-Nava force-pushed the lg7470-attempts-api-batch-job branch from 4cae04b to 46f1966 Compare November 8, 2022 17:02
@Rwolfe-Nava Rwolfe-Nava merged commit f148121 into main Nov 8, 2022
@Rwolfe-Nava Rwolfe-Nava deleted the lg7470-attempts-api-batch-job branch November 8, 2022 17:41
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