Closed awead closed 9 years ago
Can't do a granular review today but on summary perusal it looks OK to me!
I'm skeptical about this. Could your logs eat up all the memory? If you used ActiveSupport::Notifications you could subscribe to the events and write to either a log file or your in-memory struct (and then to_json)
On the other-hand I don't want to hold up this PR if it's working for you.
@jcoyne the original (current) implementation just writes out to a log file, so that shouldn't consume memory. On the other hand, keeping all the reports, i.e. structs and classes with their content, could consume a lot of memory especially if there are 10,000+ objects in the repo. Would ActiveSupport::Notifications alleviate that?
@awead it would give you an interface for encapsulating that. You could write your event subscriber with different strategies depending on what the requirements are. Again, I'm not saying you have to hold up this PR, I'm just making everyone aware of the implications of putting logging in memory and tools available to do something else.
@awead I finally get it. That said it may make sense to add some comments around the method both in the superclass and the implementation indicating that this is the point for inserting a different reporting mechanism.
Instead of logging errors to STDERR, return a JSON object with nested reports from each mover, representing the results of each step of the migration process for each object.
@cam156 @mjgiarlo would like your thoughts on this. There would be forthcoming PRs that use the resulting JSON report to re-run migrations for failed objects and interpret the report to give the user stats on what failed.