[Framework] simplify cache interfaces by weilerN · Pull Request #74 · v3io/scaler

weilerN · 2025-07-01T15:22:36Z

Motivation

Enable the watching operator to use the cache through its interface without needing awareness of the underlying implementation.

Root Cause

The original interfaces were designed to support a single string, which turned out to be insufficient.
This refactoring updates them to use []string instead.
Using string slices allows for a simpler and more accurate implementation of the cache, better aligned with the behavior defined in the HLD.

Description

For more details, see Jira: https://iguazio.atlassian.net/browse/NUC-502

Affected Areas

This PR is a prerequisite for NUC-498

Testing

Updated all unit tests and flow tests to reflect and validate the new behavior

Changes Made

Updated IngressHostCache to use []string instead of string for all relevant methods (Set, Delete) to reflect support for multiple functions.
Updated IngressHostsTree to also operate on []string rather than string for consistency and alignment with the cache layer.
Refactored FunctionTarget to simplify its role: only Equal() and ToSliceString() remain, as other behaviors (Add, Delete, Contains, IsSingle) were removed in favor of simplified usage patterns.

rokatyy

I'm not really sure regarding the proposed changes. Let's discuss it further offline

rokatyy · 2025-07-02T09:01:06Z

pkg/ingresscache/safetrie.go

+// NewTarget returns a Target based on the length of the input slice
+func (st *SafeTrie) NewTarget(inputs []string) (Target, error) {
+	switch len(inputs) {
+	case 1:
+		return Targets{inputs[0]}, nil
+	case 2:
+		return Targets{inputs[0], inputs[1]}, nil
+	default:
+		return nil, errors.New("unexpected input length")


Why not having singleTarget and canary? How the check will look like from resolving point of view? Every time we check single function (which is the most often case), we need to check both values for no reason, this is less friendly for long-time support of the code than previous solution that maps us to real entities, not some random slice of 2 that can be of len 1 or 2.

As discussed F2F, we decided to keep the SingleTarget.
Changes were committed here - CR comment - move to more generic - rename functionTarget to targets …

…and change naming singleTarget and CanaryTarget to just Targets

rokatyy · 2025-07-02T12:27:15Z

pkg/ingresscache/ingresscache.go

 		}

-		return errors.Wrap(err, "failed to set function name in the ingress host tree")
+		return errors.Wrap(err, "failed to set targets name in the ingress host tree")


Suggested change

return errors.Wrap(err, "failed to set targets name in the ingress host tree")

return errors.Wrap(err, "failed to set targets in the ingress host tree")

same in all places below

Fixed in CR comment - improve naming and clear comments

TomerShor

some nitpicks on comments mostly :)

TomerShor · 2025-07-02T13:07:20Z

pkg/ingresscache/types.go

 type IngressHostCache interface {
-	// Set adds a new item to the cache for the given host, path and function name. Will overwrite existing values if any
-	Set(host string, path string, function string) error
+	// Set adds a new item to the cache for the given host, path and targets name. Will overwrite existing values if any


Suggested change

// Set adds a new item to the cache for the given host, path and targets name. Will overwrite existing values if any

// Set adds a new item to the cache for the given host, path and targets. Will overwrite existing values if any

Fixed in CR comment - improve naming and clear comments

TomerShor · 2025-07-02T13:07:35Z

pkg/ingresscache/types.go

 type IngressHostsTree interface {
-	// Set sets a function for a given path. Will overwrite existing values if the path already exists
-	Set(path string, function string) error
+	// Set sets a targets for a given path. Will overwrite existing values if the path already exists


Suggested change

// Set sets a targets for a given path. Will overwrite existing values if the path already exists

// Set sets the targets for a given path. Will overwrite existing values if the path already exists

Fixed in CR comment - improve naming and clear comments

TomerShor · 2025-07-02T13:07:58Z

pkg/ingresscache/types.go


-	// Delete removes the function from the given path and deletes the deepest suffix used only by that function; does nothing if the path or function doesn't exist.
-	Delete(path string, function string) error
+	// Delete removes the targets from the given path and deletes the deepest suffix used only by that targets; does nothing if the path or targets doesn't exist.


Suggested change

// Delete removes the targets from the given path and deletes the deepest suffix used only by that targets; does nothing if the path or targets doesn't exist.

// Delete removes the targets from the given path and deletes the deepest suffix used only by these targets; does nothing if the path or targets don't exist.

Fixed in CR comment - improve naming and clear comments

TomerShor · 2025-07-02T13:08:23Z

pkg/ingresscache/types.go


-	// Get retrieves the best matching function names for a given path based on longest prefix match
-	Get(path string) (FunctionTarget, error)
+	// Get retrieves the best matching target names for a given path based on longest prefix match


Suggested change

// Get retrieves the best matching target names for a given path based on longest prefix match

// Get retrieves the best matching targets for a given path based on longest prefix match

Fixed in CR comment - improve naming and clear comments

TomerShor · 2025-07-02T13:08:49Z

pkg/ingresscache/safetrie.go


-// Set adds a function for a given path. If the path does not exist, it creates it
-func (st *SafeTrie) Set(path string, function string) error {
+// Set adds a targets for a given path. If the path does not exist, it creates it


Suggested change

// Set adds a targets for a given path. If the path does not exist, it creates it

// Set adds targets for a given path. If the path does not exist, it creates it

Fixed in CR comment - improve naming and clear comments

TomerShor · 2025-07-02T13:09:24Z

pkg/ingresscache/safetrie.go


-// Delete removes a function from a path and cleans up the longest suffix of the path only used by that function
-func (st *SafeTrie) Delete(path string, function string) error {
+// Delete removes a targets from a path and cleans up the longest suffix of the path only used by that targets


Suggested change

// Delete removes a targets from a path and cleans up the longest suffix of the path only used by that targets

// Delete removes the targets from a path and cleans up the longest suffix of the path only used by these targets

Fixed in CR comment - improve naming and clear comments

TomerShor · 2025-07-02T13:10:05Z

pkg/ingresscache/safetrie.go

-			st.pathTrie.Delete(path)
-		}
-		return nil
+	requestTarget, err := st.NewTarget(targets)


Suggested change

requestTarget, err := st.NewTarget(targets)

targetToDelete, err := st.NewTarget(targets)

Fixed in CR comment - improve naming and clear comments

TomerShor · 2025-07-02T13:11:29Z

pkg/ingresscache/ingresscache_test.go


 func (suite *IngressCacheTestSuite) TestAllThreeMainFunctionalitiesWithTheSameHostAndPath() {
-	// This test verifies the flow of setting a function name in an empty IngressCache, then getting it, and finally deleting it.
+	// This test verifies the flow of setting a targets name in an empty IngressCache, then getting it, and finally deleting it.


Suggested change

// This test verifies the flow of setting a targets name in an empty IngressCache, then getting it, and finally deleting it.

// This test verifies the flow of setting targets in an empty IngressCache, then getting it, and finally deleting it.

Fixed in CR comment - improve naming and clear comments

TomerShor · 2025-07-02T13:11:37Z

pkg/ingresscache/ingresscache_test.go

-	// Set a function name in an empty cache
-	err = testIngressCache.Set("example.com", "/test/path", "test-function-name-1")
-	suite.Require().NoError(err, "Expected no error when setting a function name in an empty cache")
+	// Set a targets name in an empty cache


Suggested change

// Set a targets name in an empty cache

// Set targets in an empty cache

Fixed in CR comment - improve naming and clear comments

rokatyy

LGTM

weilerN requested review from TomerShor and rokatyy July 1, 2025 15:22

weilerN added 3 commits July 1, 2025 18:24

Support string slice in cache interfaces and simplify the FunctionTarget

0dda9ee

Implementation for the interfaces changes

308b22d

Update unit tests

34a4311

weilerN force-pushed the nuc-502-simplify-cache-interfaces branch from 7c5a4b4 to 34a4311 Compare July 1, 2025 15:24

weilerN marked this pull request as ready for review July 1, 2025 15:26

weilerN marked this pull request as draft July 2, 2025 06:56

rokatyy requested changes Jul 2, 2025

View reviewed changes

CR comment - move to more generic - rename functionTarget to targets …

247f643

…and change naming singleTarget and CanaryTarget to just Targets

weilerN force-pushed the nuc-502-simplify-cache-interfaces branch from 9837dcb to 247f643 Compare July 2, 2025 11:54

weilerN marked this pull request as ready for review July 2, 2025 12:10

weilerN requested a review from rokatyy July 2, 2025 12:10

rokatyy reviewed Jul 2, 2025

View reviewed changes

TomerShor requested changes Jul 2, 2025

View reviewed changes

CR comment - improve naming and clear comments

aca2726

weilerN requested review from TomerShor and rokatyy July 2, 2025 13:38

rokatyy approved these changes Jul 2, 2025

View reviewed changes

TomerShor approved these changes Jul 3, 2025

View reviewed changes

TomerShor merged commit 5b0d9bc into v3io:development Jul 3, 2025
3 checks passed

	return errors.Wrap(err, "failed to set targets name in the ingress host tree")
	return errors.Wrap(err, "failed to set targets in the ingress host tree")

	// Set adds a new item to the cache for the given host, path and targets name. Will overwrite existing values if any
	// Set adds a new item to the cache for the given host, path and targets. Will overwrite existing values if any

	// Set sets a targets for a given path. Will overwrite existing values if the path already exists
	// Set sets the targets for a given path. Will overwrite existing values if the path already exists

	// Delete removes the targets from the given path and deletes the deepest suffix used only by that targets; does nothing if the path or targets doesn't exist.
	// Delete removes the targets from the given path and deletes the deepest suffix used only by these targets; does nothing if the path or targets don't exist.

	// Get retrieves the best matching target names for a given path based on longest prefix match
	// Get retrieves the best matching targets for a given path based on longest prefix match

	// Set adds a targets for a given path. If the path does not exist, it creates it
	// Set adds targets for a given path. If the path does not exist, it creates it

	// Delete removes a targets from a path and cleans up the longest suffix of the path only used by that targets
	// Delete removes the targets from a path and cleans up the longest suffix of the path only used by these targets

	requestTarget, err := st.NewTarget(targets)
	targetToDelete, err := st.NewTarget(targets)

	// This test verifies the flow of setting a targets name in an empty IngressCache, then getting it, and finally deleting it.
	// This test verifies the flow of setting targets in an empty IngressCache, then getting it, and finally deleting it.

	// Set a targets name in an empty cache
	// Set targets in an empty cache

Conversation

weilerN commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Root Cause

Description

Affected Areas

Testing

Changes Made

Uh oh!

rokatyy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rokatyy Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomerShor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rokatyy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

weilerN commented Jul 1, 2025 •

edited

Loading

rokatyy Jul 2, 2025 •

edited

Loading