From 8463e6a3fee748f32b13da357ca95d587a4df4f3 Mon Sep 17 00:00:00 2001 From: John McDole Date: Tue, 13 May 2025 16:13:28 -0700 Subject: [PATCH 1/2] feat: diff the configs and output them Side effect: don't set them if they aren't changing... shouldn't really be a side effect since we're single threaded. --- app_dart/lib/src/service/config.dart | 43 ++++++++++++++++--- .../service/dynamic_config_updater_test.dart | 38 ++++++++++++++-- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/app_dart/lib/src/service/config.dart b/app_dart/lib/src/service/config.dart index 3b6376468..7d1f48413 100644 --- a/app_dart/lib/src/service/config.dart +++ b/app_dart/lib/src/service/config.dart @@ -569,7 +569,7 @@ class DynamicConfigUpdater { void stopUpdateLoop() { if (_status != UpdaterStatus.running) return; - log.info('Stopping config update loop...'); + log.info('ConfigUpdater: Stopping config update loop...'); _status = UpdaterStatus.stopping; } @@ -577,7 +577,7 @@ class DynamicConfigUpdater { if (_status != UpdaterStatus.stopped) return; _status = UpdaterStatus.running; - log.info('Starting config update loop...'); + log.info('ConfigUpdater: Starting config update loop...'); // What we've decided: // 1. Each instance will **start** with a valid DynamicConfig @@ -589,18 +589,51 @@ class DynamicConfigUpdater { _delay + Duration(milliseconds: _random.nextInt(1000)), ); if (_status != UpdaterStatus.running) { - log.info('Stopped config update loop'); + log.info('ConfigUpdater: Stopped config update loop'); _status = UpdaterStatus.stopped; return; } try { final dynamicConfig = await fetchDynamicConfig(); - config._dynamicConfig = dynamicConfig; + final diffs = diffConfigChanges( + config._dynamicConfig.toJson(), + dynamicConfig.toJson(), + ); + if (diffs.isNotEmpty) { + log.info('ConfigUpdater: ${diffs.join(',')}'); + config._dynamicConfig = dynamicConfig; + } } catch (e, s) { - log.error('Unable to fetch DynamicConfig!', e, s); + log.error('ConfigUpdater: Unable to fetch DynamicConfig!', e, s); } } } + + /// Produce a simple diff of the changing flags. + List diffConfigChanges( + Map oldFlags, + Map newFlags, { + List? diffs, + String chain = 'flags', + }) { + diffs ??= []; + + for (final MapEntry(:key, :value) in oldFlags.entries) { + if (value is Map) { + diffConfigChanges( + value as Map, + newFlags[key] as Map, + diffs: diffs, + chain: '$chain.$key', + ); + continue; + } + if (value != newFlags[key]) { + diffs.add('$chain.$key $value -> ${newFlags[key]}'); + } + } + return diffs; + } } enum UpdaterStatus { stopped, running, stopping } diff --git a/app_dart/test/service/dynamic_config_updater_test.dart b/app_dart/test/service/dynamic_config_updater_test.dart index 6de740b44..0967fbad6 100644 --- a/app_dart/test/service/dynamic_config_updater_test.dart +++ b/app_dart/test/service/dynamic_config_updater_test.dart @@ -75,7 +75,7 @@ void main() { bufferedLoggerOf( containsOnce( logThat( - message: equals('Starting config update loop...'), + message: equals('ConfigUpdater: Starting config update loop...'), severity: atMostInfo, ), ), @@ -86,7 +86,7 @@ void main() { bufferedLoggerOf( containsOnce( logThat( - message: equals('Stopping config update loop...'), + message: equals('ConfigUpdater: Stopping config update loop...'), severity: atMostInfo, ), ), @@ -97,7 +97,7 @@ void main() { bufferedLoggerOf( containsOnce( logThat( - message: equals('Stopped config update loop'), + message: equals('ConfigUpdater: Stopped config update loop'), severity: atMostInfo, ), ), @@ -131,6 +131,29 @@ void main() { ); expect(config.flags.backfillerCommitLimit, 100); }); + + test('handles updating values', () async { + updater.startUpdateLoop(config); + await Future.delayed(const Duration(milliseconds: 50)); + mockHttpFile = updatedConfigYaml; + await Future.delayed(const Duration(milliseconds: 50)); + updater.stopUpdateLoop(); + + expect(config.flags.backfillerCommitLimit, 200); + expect( + log, + bufferedLoggerOf( + containsOnce( + logThat( + message: equals( + 'ConfigUpdater: flags.backfillerCommitLimit 100 -> 200', + ), + severity: atMostInfo, + ), + ), + ), + ); + }); } final class _FakeRandom extends Fake implements Random { @@ -149,3 +172,12 @@ const goodConfigYaml = ''' backfillerCommitLimit: 100 '''; + +const updatedConfigYaml = ''' +# Defines the config options for Flutter CI (Cocoon) +# +# The schema for this file is defined in DynamicConfig of +# app_dart/lib/src/service/config.dart + +backfillerCommitLimit: 200 +'''; From beb85fa2488c8f06a8359c02bcc63d56a8ad0e44 Mon Sep 17 00:00:00 2001 From: John McDole Date: Tue, 13 May 2025 16:18:23 -0700 Subject: [PATCH 2/2] Fix in flight conflict --- .../lib/src/request_handlers/scheduler/batch_backfiller.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app_dart/lib/src/request_handlers/scheduler/batch_backfiller.dart b/app_dart/lib/src/request_handlers/scheduler/batch_backfiller.dart index 836732d1d..2c42096c9 100644 --- a/app_dart/lib/src/request_handlers/scheduler/batch_backfiller.dart +++ b/app_dart/lib/src/request_handlers/scheduler/batch_backfiller.dart @@ -65,7 +65,7 @@ final class BatchBackfiller extends RequestHandler { Future _backfillExperimentalBranch(RepositorySlug slug) async { final fsGrid = await _firestore.queryRecentCommitsAndTasks( slug, - commitLimit: config.backfillerCommitLimit, + commitLimit: config.flags.backfillerCommitLimit, branch: 'ios-experimental', ); await _doBackfillFrom(slug, fsGrid, forceLowPriority: true);