From 839eb3aa66074a9bebeadec9cfafd32107df43e0 Mon Sep 17 00:00:00 2001 From: John McDole Date: Tue, 13 May 2025 15:11:17 -0700 Subject: [PATCH 1/2] feat: breakout dynamic into flags/* - stops making top-level flags in config (no one wants 100 flags to sort through) - creates a new group for content-aware-hashing - updated the current flag to show it working --- .../scheduler/batch_backfiller.dart | 4 +-- .../scheduler/vacuum_stale_tasks.dart | 2 +- app_dart/lib/src/service/config.dart | 23 +++++++++------- app_dart/lib/src/service/config.g.dart | 8 +++++- .../flags/content_aware_hashing_flags.dart | 26 +++++++++++++++++++ .../flags/content_aware_hashing_flags.g.dart | 19 ++++++++++++++ .../scheduler/batch_backfiller_test.dart | 6 ++--- .../service/dynamic_config_updater_test.dart | 2 +- app_dart/test/src/fake_config.dart | 13 +++++----- app_dart/test/src/utilities/mocks.mocks.dart | 19 ++++++++------ 10 files changed, 90 insertions(+), 32 deletions(-) create mode 100644 app_dart/lib/src/service/flags/content_aware_hashing_flags.dart create mode 100644 app_dart/lib/src/service/flags/content_aware_hashing_flags.g.dart 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 53eb07fdd6..7337105d7c 100644 --- a/app_dart/lib/src/request_handlers/scheduler/batch_backfiller.dart +++ b/app_dart/lib/src/request_handlers/scheduler/batch_backfiller.dart @@ -63,7 +63,7 @@ final class BatchBackfiller extends RequestHandler { } final fsGrid = await _firestore.queryRecentCommitsAndTasks( slug, - commitLimit: config.backfillerCommitLimit, + commitLimit: config.flags.backfillerCommitLimit, branch: branch.reference, ); await _doBackfillFrom(slug, fsGrid); @@ -74,7 +74,7 @@ final class BatchBackfiller extends RequestHandler { log.debug('Running default branch backfiller for "$slug"'); final fsGrid = await _firestore.queryRecentCommitsAndTasks( slug, - commitLimit: config.backfillerCommitLimit, + commitLimit: config.flags.backfillerCommitLimit, branch: Config.defaultBranch(slug), ); return await _doBackfillFrom(slug, fsGrid); diff --git a/app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart b/app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart index 69d74e7fa3..b5f74191cb 100644 --- a/app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart +++ b/app_dart/lib/src/request_handlers/scheduler/vacuum_stale_tasks.dart @@ -69,7 +69,7 @@ final class VacuumStaleTasks extends RequestHandler { final recentCommits = await _firestore.queryRecentCommitsAndTasks( slug, - commitLimit: config.backfillerCommitLimit, + commitLimit: config.flags.backfillerCommitLimit, status: TaskStatus.inProgress, branch: branch, ); diff --git a/app_dart/lib/src/service/config.dart b/app_dart/lib/src/service/config.dart index 3c3c258534..9e47360f4a 100644 --- a/app_dart/lib/src/service/config.dart +++ b/app_dart/lib/src/service/config.dart @@ -20,6 +20,7 @@ import 'package:yaml/yaml.dart' show YamlMap, loadYaml; import '../../cocoon_service.dart'; import '../foundation/providers.dart' show Providers; import '../foundation/typedefs.dart' show HttpClientProvider; +import 'flags/content_aware_hashing_flags.dart'; import 'github_service.dart'; import 'luci_build_service/cipd_version.dart'; @@ -33,6 +34,9 @@ interface class Config { Config(this._cache, this._secrets, {required DynamicConfig dynamicConfig}) : _dynamicConfig = dynamicConfig; + /// Access dynamically configured flags. + DynamicConfig get flags => _dynamicConfig; + /// When present on a pull request, instructs Cocoon to submit it /// automatically as soon as all the required checks pass. /// @@ -211,12 +215,6 @@ interface class Config { /// the next API call. int get backfillerTargetLimit => 75; - /// Upper limit of commit rows to be backfilled in API call. - /// - /// This limits the number of commits to be checked to backfill. When bots - /// are idle, we hope to scan as many commit rows as possible. - int get backfillerCommitLimit => _dynamicConfig.backfillerCommitLimit; - /// Upper limit of issue/PRs allowed each API call. /// /// GitHub enforces a secondary rate limit on frequency API calls. This causes @@ -504,7 +502,7 @@ interface class Config { /// /// Should be read from git/HEAD/app_dart/config.yaml and cached between /// services. -@JsonSerializable() +@JsonSerializable(explicitToJson: true) @immutable final class DynamicConfig { /// Upper limit of commit rows to be backfilled in API call. @@ -514,12 +512,17 @@ final class DynamicConfig { @JsonKey(defaultValue: 50) final int backfillerCommitLimit; - DynamicConfig({required this.backfillerCommitLimit}); + final ContentAwareHasingJson contentAwareHashing; + + DynamicConfig({ + required this.backfillerCommitLimit, + required this.contentAwareHashing, + }); /// Connect the generated [_$DynamicConfigFromJson] function to the `fromJson` /// factory. - factory DynamicConfig.fromJson(Map json) => - _$DynamicConfigFromJson(json); + factory DynamicConfig.fromJson(Map? json) => + _$DynamicConfigFromJson(json ?? {}); /// Connect the generated [_$DynamicConfigToJson] function to the `toJson` method. Map toJson() => _$DynamicConfigToJson(this); diff --git a/app_dart/lib/src/service/config.g.dart b/app_dart/lib/src/service/config.g.dart index 69d427ab69..a9c24d2aa3 100644 --- a/app_dart/lib/src/service/config.g.dart +++ b/app_dart/lib/src/service/config.g.dart @@ -12,7 +12,13 @@ DynamicConfig _$DynamicConfigFromJson(Map json) => DynamicConfig( backfillerCommitLimit: (json['backfillerCommitLimit'] as num?)?.toInt() ?? 50, + contentAwareHashing: ContentAwareHasingJson.fromJson( + json['contentAwareHashing'] as Map?, + ), ); Map _$DynamicConfigToJson(DynamicConfig instance) => - {'backfillerCommitLimit': instance.backfillerCommitLimit}; + { + 'backfillerCommitLimit': instance.backfillerCommitLimit, + 'contentAwareHashing': instance.contentAwareHashing.toJson(), + }; diff --git a/app_dart/lib/src/service/flags/content_aware_hashing_flags.dart b/app_dart/lib/src/service/flags/content_aware_hashing_flags.dart new file mode 100644 index 0000000000..7f62fec6a6 --- /dev/null +++ b/app_dart/lib/src/service/flags/content_aware_hashing_flags.dart @@ -0,0 +1,26 @@ +// Copyright 2025 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:json_annotation/json_annotation.dart'; +import 'package:meta/meta.dart'; + +part 'content_aware_hashing_flags.g.dart'; + +@JsonSerializable() +@immutable +final class ContentAwareHasingJson { + ContentAwareHasingJson({required this.waitOnContentHash}); + + /// Merge Groups should wait for the content hash before scheduling. + @JsonKey(defaultValue: false) + final bool waitOnContentHash; + + /// Connect the generated [_$ContentAwareHasingJsonFromJson] function to the `fromJson` + /// factory. + factory ContentAwareHasingJson.fromJson(Map? json) => + _$ContentAwareHasingJsonFromJson(json ?? {}); + + /// Connect the generated [_$ContentAwareHasingJsonToJson] function to the `toJson` method. + Map toJson() => _$ContentAwareHasingJsonToJson(this); +} diff --git a/app_dart/lib/src/service/flags/content_aware_hashing_flags.g.dart b/app_dart/lib/src/service/flags/content_aware_hashing_flags.g.dart new file mode 100644 index 0000000000..c551ea94f0 --- /dev/null +++ b/app_dart/lib/src/service/flags/content_aware_hashing_flags.g.dart @@ -0,0 +1,19 @@ +// GENERATED CODE - DO NOT MODIFY BY HAND + +// ignore_for_file: always_specify_types, implicit_dynamic_parameter + +part of 'content_aware_hashing_flags.dart'; + +// ************************************************************************** +// JsonSerializableGenerator +// ************************************************************************** + +ContentAwareHasingJson _$ContentAwareHasingJsonFromJson( + Map json, +) => ContentAwareHasingJson( + waitOnContentHash: json['waitOnContentHash'] as bool? ?? false, +); + +Map _$ContentAwareHasingJsonToJson( + ContentAwareHasingJson instance, +) => {'waitOnContentHash': instance.waitOnContentHash}; diff --git a/app_dart/test/request_handlers/scheduler/batch_backfiller_test.dart b/app_dart/test/request_handlers/scheduler/batch_backfiller_test.dart index 930a337256..4ddcff5114 100644 --- a/app_dart/test/request_handlers/scheduler/batch_backfiller_test.dart +++ b/app_dart/test/request_handlers/scheduler/batch_backfiller_test.dart @@ -48,9 +48,9 @@ void main() { mockGithubChecksUtil = MockGithubChecksUtil(); firestore = FakeFirestoreService(); config = FakeConfig( - backfillerCommitLimitValue: 10, backfillerTargetLimitValue: 100, supportedReposValue: {Config.flutterSlug}, + dynamicConfig: DynamicConfig.fromJson({'backfillerCommitLimit': 10}), ); ciYamlFetcher = FakeCiYamlFetcher(); @@ -100,7 +100,7 @@ void main() { }) async { final grid = await firestore.queryRecentCommitsAndTasks( Config.flutterSlug, - commitLimit: commits ?? config.backfillerCommitLimit, + commitLimit: commits ?? config.flags.backfillerCommitLimit, branch: branch, ); @@ -270,7 +270,7 @@ void main() { }); test('only considers the top X commits', () async { - config.backfillerCommitLimitValue = 1; + config.dynamicConfig = DynamicConfig.fromJson({'backfillerCommitLimit': 1}); // dart format off await fillStorageAndSetCiYaml([ diff --git a/app_dart/test/service/dynamic_config_updater_test.dart b/app_dart/test/service/dynamic_config_updater_test.dart index 836151637d..6de740b447 100644 --- a/app_dart/test/service/dynamic_config_updater_test.dart +++ b/app_dart/test/service/dynamic_config_updater_test.dart @@ -129,7 +129,7 @@ void main() { '${requestUris.last}', 'https://raw.githubusercontent.com/flutter/cocoon/main/app_dart/config.yaml', ); - expect(config.backfillerCommitLimit, 100); + expect(config.flags.backfillerCommitLimit, 100); }); } diff --git a/app_dart/test/src/fake_config.dart b/app_dart/test/src/fake_config.dart index 90ea11bc40..9c25b284e3 100644 --- a/app_dart/test/src/fake_config.dart +++ b/app_dart/test/src/fake_config.dart @@ -49,11 +49,11 @@ class FakeConfig implements Config { this.supportedReposValue, this.batchSizeValue, this.backfillerTargetLimitValue, - this.backfillerCommitLimitValue, this.issueAndPRLimitValue, this.githubRequestDelayValue, DynamicConfig? dynamicConfig, - }); + }) : dynamicConfig = + dynamicConfig ?? DynamicConfig.fromJson({}); gh.GitHub? githubClient; GraphQLClient? githubGraphQLClient; @@ -75,7 +75,6 @@ class FakeConfig implements Config { String? releaseCandidateBranchPathValue; Set? rollerAccountsValue; int? backfillerTargetLimitValue; - int? backfillerCommitLimitValue; int? issueAndPRLimitValue; String? flutterGoldPendingValue; String? flutterGoldSuccessValue; @@ -89,6 +88,7 @@ class FakeConfig implements Config { Set? supportedReposValue; Set? postsubmitSupportedReposValue; Duration? githubRequestDelayValue; + DynamicConfig dynamicConfig; @override Future createGitHubClient({ @@ -120,9 +120,6 @@ class FakeConfig implements Config { @override int get backfillerTargetLimit => backfillerTargetLimitValue ?? 50; - @override - int get backfillerCommitLimit => backfillerCommitLimitValue ?? 50; - @override int get issueAndPRLimit => issueAndPRLimitValue ?? 2; @@ -268,4 +265,8 @@ class FakeConfig implements Config { @override Future get discordTreeStatusWebhookUrl async => 'https://discord.com/api/webhooks/1234/abcd'; + + @override + // TODO: implement flags + DynamicConfig get flags => dynamicConfig; } diff --git a/app_dart/test/src/utilities/mocks.mocks.dart b/app_dart/test/src/utilities/mocks.mocks.dart index 219688e782..688f25a774 100644 --- a/app_dart/test/src/utilities/mocks.mocks.dart +++ b/app_dart/test/src/utilities/mocks.mocks.dart @@ -736,6 +736,17 @@ class MockConfig extends _i1.Mock implements _i2.Config { ) as Set<_i7.RepositorySlug>); + @override + _i2.DynamicConfig get flags => + (super.noSuchMethod( + Invocation.getter(#flags), + returnValue: _i20.dummyValue<_i2.DynamicConfig>( + this, + Invocation.getter(#flags), + ), + ) + as _i2.DynamicConfig); + @override Set<_i7.RepositorySlug> get supportedRepos => (super.noSuchMethod( @@ -852,14 +863,6 @@ class MockConfig extends _i1.Mock implements _i2.Config { ) as int); - @override - int get backfillerCommitLimit => - (super.noSuchMethod( - Invocation.getter(#backfillerCommitLimit), - returnValue: 0, - ) - as int); - @override int get issueAndPRLimit => (super.noSuchMethod(Invocation.getter(#issueAndPRLimit), returnValue: 0) From ef531cd17332c57b46d0fd71c2cb2de0f43eea0e Mon Sep 17 00:00:00 2001 From: John McDole Date: Tue, 13 May 2025 15:52:39 -0700 Subject: [PATCH 2/2] Hashing --- app_dart/lib/src/service/config.dart | 2 +- app_dart/lib/src/service/config.g.dart | 2 +- .../service/flags/content_aware_hashing_flags.dart | 14 +++++++------- .../flags/content_aware_hashing_flags.g.dart | 8 ++++---- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app_dart/lib/src/service/config.dart b/app_dart/lib/src/service/config.dart index 9e47360f4a..3b63764685 100644 --- a/app_dart/lib/src/service/config.dart +++ b/app_dart/lib/src/service/config.dart @@ -512,7 +512,7 @@ final class DynamicConfig { @JsonKey(defaultValue: 50) final int backfillerCommitLimit; - final ContentAwareHasingJson contentAwareHashing; + final ContentAwareHashingJson contentAwareHashing; DynamicConfig({ required this.backfillerCommitLimit, diff --git a/app_dart/lib/src/service/config.g.dart b/app_dart/lib/src/service/config.g.dart index a9c24d2aa3..9ef740ed48 100644 --- a/app_dart/lib/src/service/config.g.dart +++ b/app_dart/lib/src/service/config.g.dart @@ -12,7 +12,7 @@ DynamicConfig _$DynamicConfigFromJson(Map json) => DynamicConfig( backfillerCommitLimit: (json['backfillerCommitLimit'] as num?)?.toInt() ?? 50, - contentAwareHashing: ContentAwareHasingJson.fromJson( + contentAwareHashing: ContentAwareHashingJson.fromJson( json['contentAwareHashing'] as Map?, ), ); diff --git a/app_dart/lib/src/service/flags/content_aware_hashing_flags.dart b/app_dart/lib/src/service/flags/content_aware_hashing_flags.dart index 7f62fec6a6..584e61a90f 100644 --- a/app_dart/lib/src/service/flags/content_aware_hashing_flags.dart +++ b/app_dart/lib/src/service/flags/content_aware_hashing_flags.dart @@ -9,18 +9,18 @@ part 'content_aware_hashing_flags.g.dart'; @JsonSerializable() @immutable -final class ContentAwareHasingJson { - ContentAwareHasingJson({required this.waitOnContentHash}); +final class ContentAwareHashingJson { + ContentAwareHashingJson({required this.waitOnContentHash}); /// Merge Groups should wait for the content hash before scheduling. @JsonKey(defaultValue: false) final bool waitOnContentHash; - /// Connect the generated [_$ContentAwareHasingJsonFromJson] function to the `fromJson` + /// Connect the generated [_$ContentAwareHashingJsonFromJson] function to the `fromJson` /// factory. - factory ContentAwareHasingJson.fromJson(Map? json) => - _$ContentAwareHasingJsonFromJson(json ?? {}); + factory ContentAwareHashingJson.fromJson(Map? json) => + _$ContentAwareHashingJsonFromJson(json ?? {}); - /// Connect the generated [_$ContentAwareHasingJsonToJson] function to the `toJson` method. - Map toJson() => _$ContentAwareHasingJsonToJson(this); + /// Connect the generated [_$ContentAwareHashingJsonToJson] function to the `toJson` method. + Map toJson() => _$ContentAwareHashingJsonToJson(this); } diff --git a/app_dart/lib/src/service/flags/content_aware_hashing_flags.g.dart b/app_dart/lib/src/service/flags/content_aware_hashing_flags.g.dart index c551ea94f0..dd6008b444 100644 --- a/app_dart/lib/src/service/flags/content_aware_hashing_flags.g.dart +++ b/app_dart/lib/src/service/flags/content_aware_hashing_flags.g.dart @@ -8,12 +8,12 @@ part of 'content_aware_hashing_flags.dart'; // JsonSerializableGenerator // ************************************************************************** -ContentAwareHasingJson _$ContentAwareHasingJsonFromJson( +ContentAwareHashingJson _$ContentAwareHashingJsonFromJson( Map json, -) => ContentAwareHasingJson( +) => ContentAwareHashingJson( waitOnContentHash: json['waitOnContentHash'] as bool? ?? false, ); -Map _$ContentAwareHasingJsonToJson( - ContentAwareHasingJson instance, +Map _$ContentAwareHashingJsonToJson( + ContentAwareHashingJson instance, ) => {'waitOnContentHash': instance.waitOnContentHash};