Merge branch 'sc1-manifest-import-metadata-use-redis-hash' into 'master'

Use hash tags for Redis keys in ManifestImport::Metadata

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124871



Merged-by: Luke Duncalfe <lduncalfe@gitlab.com>
Approved-by: Luke Duncalfe <lduncalfe@gitlab.com>
Reviewed-by: Bob Van Landuyt <bob@gitlab.com>
Co-authored-by: Bob Van Landuyt <bob@gitlab.com>
Co-authored-by: Sylvester Chin <schin@gitlab.com>

config/feature_flags/development/manifest_import_use_hash_tagged_key.yml

+---
+name: manifest_import_use_hash_tagged_key
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124871
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/416710
+milestone: '16.2'
+type: development
+group: group::scalability
+default_enabled: false

lib/gitlab/manifest_import/metadata.rb

@@ -4,6 +4,7 @@ module Gitlab
   module ManifestImport
     class Metadata
       EXPIRY_TIME = 1.week
+      KEY_PREFIX = 'manifest_import:metadata:user'
 
       attr_reader :user, :fallback
 
@@ -13,6 +14,15 @@ def initialize(user, fallback: {})
       end
 
       def save(repositories, group_id)
+        if Feature.enabled?(:manifest_import_use_hash_tagged_key)
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.multi do |multi|
+              multi.set(hashtag_key_for('repositories'), Gitlab::Json.dump(repositories), ex: EXPIRY_TIME)
+              multi.set(hashtag_key_for('group_id'), group_id, ex: EXPIRY_TIME)
+            end
+          end
+        end
+
         Gitlab::Redis::SharedState.with do |redis|
           Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
             redis.multi do |multi|
@@ -37,13 +47,21 @@ def group_id
 
       private
 
+      def hashtag_key_for(field)
+        "#{KEY_PREFIX}:{#{user.id}}:#{field}"
+      end
+
       def key_for(field)
-        "manifest_import:metadata:user:#{user.id}:#{field}"
+        "#{KEY_PREFIX}:#{user.id}:#{field}"
       end
 
       def redis_get(field)
         Gitlab::Redis::SharedState.with do |redis|
-          redis.get(key_for(field))
+          if Feature.enabled?(:manifest_import_use_hash_tagged_key)
+            redis.get(hashtag_key_for(field)) || redis.get(key_for(field))
+          else
+            redis.get(key_for(field))
+          end
         end
       end
     end

spec/lib/gitlab/manifest_import/metadata_spec.rb

@@ -12,51 +12,98 @@
   end
 
   describe '#save' do
-    it 'stores data in Redis with an expiry of EXPIRY_TIME' do
-      status = described_class.new(user)
-      repositories_key = 'manifest_import:metadata:user:1:repositories'
-      group_id_key = 'manifest_import:metadata:user:1:group_id'
+    let(:status) { described_class.new(user) }
+    let(:hashtag_repositories_key) { 'manifest_import:metadata:user:{1}:repositories' }
+    let(:hashtag_group_id_key) { 'manifest_import:metadata:user:{1}:group_id' }
+    let(:repositories_key) { 'manifest_import:metadata:user:1:repositories' }
+    let(:group_id_key) { 'manifest_import:metadata:user:1:group_id' }
+
+    subject { status.save(repositories, 2) }
 
-      status.save(repositories, 2)
+    it 'stores data in Redis with an expiry of EXPIRY_TIME' do
+      subject
 
       Gitlab::Redis::SharedState.with do |redis|
+        expect(redis.ttl(hashtag_repositories_key)).to be_within(5).of(described_class::EXPIRY_TIME)
+        expect(redis.ttl(hashtag_group_id_key)).to be_within(5).of(described_class::EXPIRY_TIME)
         expect(redis.ttl(repositories_key)).to be_within(5).of(described_class::EXPIRY_TIME)
         expect(redis.ttl(group_id_key)).to be_within(5).of(described_class::EXPIRY_TIME)
       end
     end
+
+    context 'with `manifest_import_use_hash_tagged_key`feature flag disabled' do
+      before do
+        stub_feature_flags(manifest_import_use_hash_tagged_key: false)
+      end
+
+      it 'only stores Redis String' do
+        subject
+
+        Gitlab::Redis::SharedState.with do |redis|
+          expect(redis.ttl(hashtag_repositories_key)).to eq(-2)
+          expect(redis.ttl(hashtag_group_id_key)).to eq(-2)
+          expect(redis.ttl(repositories_key)).to be_within(5).of(described_class::EXPIRY_TIME)
+          expect(redis.ttl(group_id_key)).to be_within(5).of(described_class::EXPIRY_TIME)
+        end
+      end
+    end
   end
 
   describe '#repositories' do
-    it 'allows repositories to round-trip with symbol keys' do
-      status = described_class.new(user)
+    shared_examples 'read repositories' do
+      it 'allows repositories to round-trip with symbol keys' do
+        status = described_class.new(user)
 
-      status.save(repositories, 2)
+        status.save(repositories, 2)
+
+        expect(status.repositories).to eq(repositories)
+      end
 
-      expect(status.repositories).to eq(repositories)
+      it 'uses the fallback when there is nothing in Redis' do
+        fallback = { manifest_import_repositories: repositories }
+        status = described_class.new(user, fallback: fallback)
+
+        expect(status.repositories).to eq(repositories)
+      end
     end
 
-    it 'uses the fallback when there is nothing in Redis' do
-      fallback = { manifest_import_repositories: repositories }
-      status = described_class.new(user, fallback: fallback)
+    context 'with `manifest_import_use_hash_tagged_key` feature flag disabled' do
+      before do
+        stub_feature_flags(manifest_import_use_hash_tagged_key: false)
+      end
 
-      expect(status.repositories).to eq(repositories)
+      it_behaves_like 'read repositories'
     end
+
+    it_behaves_like 'read repositories'
   end
 
   describe '#group_id' do
-    it 'returns the group ID as an integer' do
-      status = described_class.new(user)
+    shared_examples 'read group_id' do
+      it 'returns the group ID as an integer' do
+        status = described_class.new(user)
+
+        status.save(repositories, 2)
 
-      status.save(repositories, 2)
+        expect(status.group_id).to eq(2)
+      end
+
+      it 'uses the fallback when there is nothing in Redis' do
+        fallback = { manifest_import_group_id: 3 }
+        status = described_class.new(user, fallback: fallback)
 
-      expect(status.group_id).to eq(2)
+        expect(status.group_id).to eq(3)
+      end
     end
 
-    it 'uses the fallback when there is nothing in Redis' do
-      fallback = { manifest_import_group_id: 3 }
-      status = described_class.new(user, fallback: fallback)
+    context 'with feature flag disabled' do
+      before do
+        stub_feature_flags(manifest_import_use_hash_tagged_key: false)
+      end
 
-      expect(status.group_id).to eq(3)
+      it_behaves_like 'read group_id'
     end
+
+    it_behaves_like 'read group_id'
   end
 end