Skip to content

Conversation

kwasniew
Copy link
Contributor

@kwasniew kwasniew commented Aug 28, 2025

About the changes

  • extracted backup file reading from a polling toggle fetcher
  • reading backup file on startup in streaming so even on unsuccessful connection we get some backup
  • also added bootstrap reading in streaming and if the bootstrap fails we fall back to backup file
  • deliberately keeping backup and bootstrap duplicated between polling and streaming to avoid premature abstraction. it was enough to extract the shared backup file reader
  • added tests describing missing behavior (tests inspired by toggle_fetcher_spec.rb)

Important files

Discussion points

We may consider reading backup file outside of the fetcher/streamer if we change current architecture a bit

@coveralls
Copy link

coveralls commented Aug 28, 2025

Pull Request Test Coverage Report for Build 17298764007

Details

  • 30 of 33 (90.91%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on read-bootstrap-and-backup-streaming at 94.617%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/unleash/backup_file_reader.rb 9 12 75.0%
Totals Coverage Status
Change from base Build 17093044303: 94.6%
Covered Lines: 580
Relevant Lines: 613

💛 - Coveralls

require 'json'

module Unleash
class BackupFileReader
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is extracted from toggle_fetcher

Copy link
Member

Choose a reason for hiding this comment

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

Nice, I really like this

self.event_processor = Unleash::StreamingEventProcessor.new(engine)
self.running = false

begin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

similar setup to toggle fetcher in polling mode but we don't make first request to the server and instead go straight to either bootstrap or a backup file. Then streaming starts and takes over from there

@kwasniew kwasniew requested a review from sighphyre August 28, 2025 14:26
@gastonfournier gastonfournier moved this from New to In Progress in Issues and PRs Aug 29, 2025
Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

Nice, I like the harvested backup file reader!

Unleash.logger.error "Unable to read the backup_file: #{e}"
# :nocov:
nil
rescue JSON::ParserError => e
Copy link
Member

Choose a reason for hiding this comment

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

I actually don't think this case can happen since we no longer parse on read. I realise this is a hangover from the old implementation (my bad, should have dropped this when I ported to Ygg). If we don't remove it in this PR, no biggie, I'll do it in a future one

read_backup_file!(engine)
end
rescue StandardError => e
# fail back to reading the backup file
Copy link
Member

Choose a reason for hiding this comment

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

Also a hangover from the old code haha, but may as well fix it while we're here

Suggested change
# fail back to reading the backup file
# fall back to reading the backup file

require 'json'

module Unleash
class BackupFileReader
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I really like this

@github-project-automation github-project-automation bot moved this from In Progress to Approved PRs in Issues and PRs Sep 3, 2025
@kwasniew kwasniew merged commit bf943d4 into main Sep 3, 2025
44 checks passed
@kwasniew kwasniew deleted the read-bootstrap-and-backup-streaming branch September 3, 2025 14:10
@github-project-automation github-project-automation bot moved this from Approved PRs to Done in Issues and PRs Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants