[Framework] Add ingress cache by weilerN · Pull Request #70 · v3io/scaler

weilerN · 2025-06-11T13:21:59Z

Motivation

Introduce efficient host+path resolution for DLX with O(logN) complexity, where N = len(strings.Split(path, "/")).
For full technical context, see HLD

Description

This PR lays the foundation for future ingress cache functionality, as described in the JIRA- https://iguazio.atlassian.net/browse/NUC-423 .
Key deliverables include:

Initial cache DB structure
Thread-safe trie (safeTrie) for efficient path resolution
Unit tests validating DB operations and trie logic

Affected Areas

This is a standalone implementation with no impact on existing functionality.
It introduces isolated, internal components to support upcoming features.

Testing

Unit tests: cover all core cache logic and pathTrie interface implementations
Integration tests: to be addressed in a dedicated follow-up task

Changes Made

Added ingressCache.go - implements IngressHostCache.
Added safeTrie.go - implements IngressHostsTree.

Additional Notes

Additional in-memory optimizations for safeTrie are planned separately to keep this PR focused and scoped.

TomerShor

Initial review, mostly related to our conventions.
Main things are:

File name are all lowercase, even for multiple words.
We use the errors package over fmt.Errorf etc.
When returning errors, think about future Noam trying to debug this code - what will be his experience?
Utilities have a dedicated space in the common package.
Private over Public - consider who's gonna use the interfaces and structs, and if they should be indeed private.

I'll focus more on the logic the next round.

TomerShor · 2025-06-11T14:03:48Z

pkg/ingressCache/ingressHostTree.go

@@ -0,0 +1,7 @@
+package ingressCache
+
+type ingressHostsTree interface {


Why is this private? will this only be used internally in the ingresscache package?
Usually interfaces are meant to be implemented by structs from different packages. If there is only one object that will implement these functions - maybe it shouldn't be an interface.

I agree that this interface could be public, but I prefer to keep it. The data structure implementing the interface is based on a third-party open-source package, and keeping the interface gives us flexibility to change or modify the package if needed.

Changed into a public interface - here

TomerShor · 2025-06-11T14:05:27Z

pkg/ingressCache/pathTrie.go

+const (
+	emptyPathError         = "path is empty"
+	emptyFunctionNameError = "function name is empty"
+	walkPathResultError    = "value is not a []string"
+	functionNotExistsError = "function does not exist in path"
+)


define error type consts instead of strings:

const ( emptyPathError error = errors.New("Path is empty") ... )

secondly - I prefer to avoid having errors as constants, and much prefer having the explicit string where the errors are returned.
It makes the code a lot more readable, and the messages easy for find in the code if you encounter them.

thirdly, we use the errors package (github.com/nuclio/errors) which provides both New and better Wrapping of errors, using errors.Wrap(err, <message>).

Thanks for the explanation. I was using const in tests to check negative flows. I’ll update my code to follow the existing convention.

Fixed- no const errors in any file

TomerShor · 2025-06-11T14:07:26Z

pkg/ingressCache/pathTrie.go

+func newPathTree() *pathTree {
+	return &pathTree{*trie.NewPathTrie()}
+}


The constructor should be after the struct definition (convention, not error).

// Definition type MyStruct struct { ... } // Constructor func NewMyStruct() *MyStruct { ... } // Methods func (*MyStruct) Get() { ... }

TomerShor · 2025-06-11T14:08:55Z

pkg/ingressCache/pathTrie.go

+	}
+
+	if function == "" {
+		return fmt.Errorf(emptyFunctionNameError)


Same for the other errors.

TomerShor · 2025-06-11T15:53:52Z

pkg/ingressCache/ingressHostTree.go

@@ -0,0 +1,7 @@
+package ingressCache


as the action says - rename package (and folder) to ingresscache

TomerShor · 2025-06-11T16:01:22Z

pkg/ingressCache/pathTrie.go

+		return fmt.Errorf(emptyFunctionNameError)
+	}
+
+	// get the exact path value in order to avoid creating a new path if it does not exist


avoid creating a new path if it does not exist

?
You do create it if it does not exist.

right, bad typo

TomerShor · 2025-06-11T16:11:40Z

pkg/ingressCache/pathTrie.go

+func excludeElemFromSlice(slice []string, elem string) []string {
+	var output []string
+	for _, v := range slice {
+		if v == elem {
+			continue
+		}
+		output = append(output, v)
+	}
+	return output
+}


Move to common/helpers.go

another suggestion here: https://stackoverflow.com/a/37335777

I will keep the function in this file because it's gonna be removed anyway as part of this improvement

I refactored the function to be as efficient as possible here

TomerShor · 2025-06-11T16:12:45Z

pkg/ingressCache/pathTrie.go

+		// TODO - add log that function does not exist in the path
+		return fmt.Errorf("%s,  function:%s, path:%s", functionNotExistsError, function, path)


is this really an error?
I would say the "Deletion" succeeded (idempotency - remember)?

Well, this won’t affect idempotency, but on second thought, I agree that this could be logged instead and shouldn’t return an error.

TomerShor · 2025-06-11T16:13:59Z

pkg/ingressCache/pathTrie.go

+// get retrieve the closest prefix matching the path and returns the associated functions
+func (p *pathTree) get(path string) ([]string, error) {
+	var walkPathResult interface{}
+	err := p.WalkPath(path, func(path string, value interface{}) error {


Suggested change

err := p.WalkPath(path, func(path string, value interface{}) error {

if err := p.WalkPath(path, func(path string, value interface{}) error {

...

}); err != nil {

...

}

TomerShor · 2025-06-11T16:14:56Z

pkg/ingressCache/pathTrie.go

+func existsInSlice(slice []string, elem string) bool {
+	for _, v := range slice {
+		if v == elem {
+			return true
+		}
+	}
+	return false
+}


no need if you use slices.contains

rokatyy

General Comments and Suggestions Regarding Tests:

Avoid using constants for dynamic values:
It’s generally better to avoid hardcoding constants for things like function names or other values that are logically variable. Using variables instead makes tests clearer and easier to maintain, especially if those values might change or differ between test cases.
Avoid mocking if possible:
In this context, since the object being tested is abstract or a simple interface, the need for mocking is limited. Using real or simple concrete instances can improve test clarity and reduce complexity. Mocking should be reserved for dependencies that are expensive, have side effects, or are external.
ensure tests are isolated and independent:
Tests should not rely on shared state or cross dependencies. This is important to guarantee that tests can run reliably in parallel or in any order without interference. Proper setup and teardown are key to avoiding flakiness and making tests robust.

rokatyy · 2025-06-16T15:22:00Z

pkg/ingressCache/ingressCache.go

@@ -0,0 +1,124 @@
+/*
+Copyright 2019 Iguazio Systems Ltd.


Suggested change

Copyright 2019 Iguazio Systems Ltd.

Copyright 2025 Iguazio Systems Ltd.

Fixed here - CR comment - Copyright

rokatyy · 2025-06-16T15:26:11Z

pkg/ingressCache/ingressCache.go

+	urlTree, exists := ic.syncMap.Load(host)
+	if !exists {
+		urlTree = NewSafeTrie()
+		ic.syncMap.Store(host, urlTree)
+	}
+


use ic.syncMap.LoadOrStore() instead

To avoid using syncMap.Delete on every failure, I chose to keep the result from syncMap.Load and only store it at the end of the positive flow.

Fixed here - CR comment - improve usage of syncMap and slice

rokatyy · 2025-06-16T15:30:25Z

pkg/ingressCache/ingressCache.go

+
+type IngressHostsTree interface {
+	SetFunctionName(path string, function string) error // will overwrite existing values if exists
+	DeleteFunctionName(path string, function string) error
+	GetFunctionName(path string) ([]string, error)
+	IsEmpty() bool
+}
+
+type IngressHostCache interface {
+	Set(host string, path string, function string) error // will overwrite existing values if exists
+	Delete(host string, path string, function string) error
+	Get(host string, path string) ([]string, error)
+}


Top lvl entity up

Suggested change

type IngressHostsTree interface {

SetFunctionName(path string, function string) error // will overwrite existing values if exists

DeleteFunctionName(path string, function string) error

GetFunctionName(path string) ([]string, error)

IsEmpty() bool

}

type IngressHostCache interface {

Set(host string, path string, function string) error // will overwrite existing values if exists

Delete(host string, path string, function string) error

Get(host string, path string) ([]string, error)

}

type IngressHostCache interface {

Set(host string, path string, function string) error // will overwrite existing values if exists

Delete(host string, path string, function string) error

Get(host string, path string) ([]string, error)

}

type IngressHostsTree interface {

SetFunctionName(path string, function string) error // will overwrite existing values if exists

DeleteFunctionName(path string, function string) error

GetFunctionName(path string) ([]string, error)

IsEmpty() bool

}

Fixed here - CR comment - rename directory and interfaces comments

rokatyy · 2025-06-16T15:32:59Z

pkg/ingressCache/ingressCache.go

+		ic.logger.DebugWith("cache delete: host removed as it is empty",
+			"host", host)
+		ic.syncMap.Delete(host)


Suggested change

ic.logger.DebugWith("cache delete: host removed as it is empty",

"host", host)

ic.syncMap.Delete(host)

ic.syncMap.Delete(host)

ic.logger.DebugWith("cache delete: host removed as it is empty",

"host", host)

rokatyy · 2025-06-16T15:37:06Z

pkg/ingressCache/ingressCache.go

+	SetFunctionName(path string, function string) error // will overwrite existing values if exists
+	DeleteFunctionName(path string, function string) error
+	GetFunctionName(path string) ([]string, error)


maybe remove name from SetFunctionName and others? So when we move to typed trie values (not [string]), it will return either canaryFunction or singleFunction

I believe we should keep using functionName — it's a commonly used convention and clearly describes the value the trie holds.
But as @TomerShor recommended here, I will change the GetFunctionName specifically.

@weilerN I think we will need to rename when code is moved from using []string to structs, which will be with function prefix in the name (e.g canaryFunction, singleFunction). Though naming can be reconsidered in the following PR

rokatyy · 2025-06-16T15:43:37Z

pkg/ingressCache/ingressCache_test.go

+		expectedResult []string
+		shouldFail     bool
+		errorMessage   string
+		testMocks      mockFunction


what is the reason of using mock here? Won't proper object creation be the better fit here?

Discuses F2F, fixed here- CR comment - refactor UTs (remove mocks, consts and setupTest)

rokatyy · 2025-06-16T15:45:13Z

pkg/ingressCache/mock/safeTrie.go

I don't really understand why mock for safeTrie is needed in general as it's in-memory data structure with no external dependencies, so using it directly in tests is fast, reliable, and easy to control.

Discussed F2F, removed here - CR comment - refactor UTs (remove mocks, consts and setupTest)

rokatyy · 2025-06-16T15:51:24Z

pkg/ingressCache/safeTrie_test.go

+	"github.com/stretchr/testify/suite"
+)
+
+type SafeTrieTest struct {


Suggested change

type SafeTrieTest struct {

type SafeTrieTestSuite struct {

rokatyy · 2025-06-16T15:54:34Z

pkg/ingressCache/safeTrie_test.go

+
+type SafeTrieTest struct {
+	suite.Suite
+	safeTrie *SafeTrie


1stly, That's not a best practice to have a shared object, which is used across all the tests, in suite. 2ndly, it fully blocks tests from parallel execution even though these are unit tests and can be executed in parallel. Please create a new safeTrie in every tests to avoid unnecessary complexity

@rokatyy I'm aware that the unit tests run in parallel, and I've verified it prior to committing the code:

Each test creates is own safeTrie before starting - link.

But I agree with you that the safeTrie should be part of the test itself and not part of the Suite so I will move it.

You just was lucky :)

Fixed here - CR comment - refactor UTs (remove mocks, consts and setupTest)

rokatyy · 2025-06-16T16:01:42Z

pkg/ingressCache/safeTrie_test.go

+	testFunctionName := "test-function"
+	testFunctionName2 := "test-function-2"
+	testFunctionPath := "/path/to/function"
+	testFunctionPathNested := "/path/to/function/nested"
+	testFunctionPathEndsWithSlash := "/path/to/function/"
+	testFunctionPathWithDots := "/path/./to/./function/"
+	testFunctionPathUpperCase := "/PATH/TO/function"
+	testFunctionNameUpperCase := "test-FUNCTION"
+	testAPIPath := "/api/v1/user-data/123"


please remove this and pass raw strings to test cases below. Also, testFunctionName1 and testFunctionName2 are consts which leads us to the reason why one should not use consts in tests especially for names if this is not really necessary.

@rokatyy I initially defined these variables within each test, but the linter raised the following error:

link

That leaves us with a few options:

Add //goconst comments to suppress the warning per test (which isn't ideal), or disable this check here

Use constants instead.

Adjust the linter configuration to ignore this check altogether.

In each test create a unique dummy variable names. This walk around the linter.

What approach do you prefer?

@weilerN use local variables, no need in global once. And these variable are not required as they can be passed as strings to test cases.

Fixed here - CR comment - refactor UTs (remove mocks, consts and setupTest)

@rokatyy As we discussed yesterday - the linter didn't fail the test when using strings inside the tests, so no need to adjust the goconst

pkg/ingressCache/ingressCache.go

TomerShor · 2025-06-16T16:19:41Z

pkg/ingressCache/ingressCache.go

+		if !exists {
+			ic.syncMap.Delete(host)
+		}
+		return errors.Wrap(err, "cache set failed")


Even more explicit:

Suggested change

return errors.Wrap(err, "cache set failed")

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

The caller of Set will return an error Failed to set function in cache.

pkg/ingressCache/safeTrie.go

TomerShor · 2025-06-16T16:49:06Z

pkg/ingressCache/safeTrie_test.go

+func (suite *SafeTrieTest) SetupTest() {
+	suite.safeTrie = NewSafeTrie()
+}
+
+func (suite *SafeTrieTest) SetupSubTest(safeTrieState []safeTrieFunctionArgs) {


when you have a suite, you can have SetupSuite / TeardownSuite for things that relate to all tests, and SetupTest / TeardownTest for specific tests stuff.

Good point, I will take that in mind.
Fixed here - CR comment - refactor UTs (remove mocks, consts and setupTest)

TomerShor

Logic looks much better!
Added some comments regarding tests

TomerShor · 2025-06-18T10:21:01Z

pkg/ingresscache/safetrie_test.go

+	return resultMap, nil
+}
+
+func (suite *SafeTrieTestSuite) generateLotsOfPathsAndFunctions(num int) []safeTrieFunctionArgs {


Suggested change

func (suite *SafeTrieTestSuite) generateLotsOfPathsAndFunctions(num int) []safeTrieFunctionArgs {

func (suite *SafeTrieTestSuite) generatePathsAndFunctions(num int) []safeTrieFunctionArgs {

TomerShor · 2025-06-18T10:26:32Z

pkg/ingresscache/safetrie_test.go

+		},
+	} {
+		suite.Run(testCase.name, func() {
+			suite.SetupSubTest(nil)


From testify's docs, you don't really need to call SetupSubTest manually, as suite.Run calls it anyway. However - it calls it without any arguments.
What you need here is a SetupTest function that only initializes suite.safeTrie.

As suggested, I removed the SetupSubTestand moved into a privategetTestSafeTrie` function.
CR comment - refactor UTs (remove mocks, consts and setupTest)

TomerShor · 2025-06-18T10:28:15Z

pkg/ingresscache/safetrie_test.go

+		suite.SetupSubTest([]safeTrieFunctionArgs{
+			{testPathRoot, testFunctionName},
+			{testPath1, testFunctionName1},
+			{testPath2, testFunctionName2},
+			{testFunctionPathWithDots, testFunctionName1},
+			{testFunctionPathWithDoubleSlash, testFunctionName1},
+			{testPathWithMultipleFunctions, testFunctionName1},
+			{testPathWithMultipleFunctions, testFunctionName2},
+		})


As my previous comment, this is not the correct usage of SetupSubTest.
You can create a helper private function that populates suite.safeTrie with data, and call it here.

As suggested, I removed the SetupSubTest and refactored to privategetTestSafeTrie function.
CR comment - refactor UTs (remove mocks, consts and setupTest)

TomerShor · 2025-06-18T10:29:31Z

pkg/ingresscache/ingresscache_test.go

+func (suite *IngressCacheTestSuite) SetupSubTest(testHost string, testMocks mockFunction) {
+	suite.ingressCache = NewIngressCache(suite.logger)
+
+	if m := testMocks(); m != nil {
+		// mock==nil is used to check for non-existing host
+		suite.ingressCache.syncMap.Store(testHost, m)
+	}
+}


See the comments in safetrie_test.go, same applies here

Fixed here CR comment - refactor UTs (remove mocks, consts and setupTest)

TomerShor · 2025-06-18T10:30:03Z

pkg/ingresscache/ingresscache_test.go

+
+			resultFunctionNames, err := suite.ingressCache.Get(testCase.args.host, testCase.args.path)
+			if testCase.shouldFail {
+				suite.Require().NotNil(err)


Suggested change

suite.Require().NotNil(err)

suite.Require().Error(err)

Fixed here CR comment - refactor UTs (remove mocks, consts and setupTest)

TomerShor · 2025-06-18T10:30:28Z

pkg/ingresscache/ingresscache_test.go

+			resultFunctionNames, err := suite.ingressCache.Get(testCase.args.host, testCase.args.path)
+			if testCase.shouldFail {
+				suite.Require().NotNil(err)
+				suite.Require().Contains(err.Error(), testCase.errorMessage)


Suggested change

suite.Require().Contains(err.Error(), testCase.errorMessage)

suite.Require().ErrorContains(err, testCase.errorMessage)

Fixed here CR comment - refactor UTs (remove mocks, consts and setupTest)

TomerShor · 2025-06-18T10:30:43Z

pkg/ingresscache/ingresscache_test.go

+				suite.Require().NotNil(err)
+				suite.Require().Contains(err.Error(), testCase.errorMessage)


Fixed here - CR comment - refactor UTs (remove mocks, consts and setupTest)

TomerShor · 2025-06-18T10:30:52Z

pkg/ingresscache/ingresscache_test.go

+				suite.Require().NotNil(err)
+				suite.Require().Contains(err.Error(), testCase.errorMessage)


Fixed here - CR comment - refactor UTs (remove mocks, consts and setupTest)

rokatyy · 2025-06-19T10:11:09Z

pkg/ingresscache/ingresscache_test.go

+		}, {
+			name:           "Get multiple functionName",
+			args:           testIngressCacheArgs{"example.com", "/test/path", ""},
+			expectedResult: []string{"test-function-name-1", "test-function-name-2"},
+			initialState: []ingressCacheTestInitialState{
+				{"example.com", "/test/path", "test-function-name-1"},
+				{"example.com", "/test/path", "test-function-name-2"},


how is this different from Get two functionName?

Good catch, both test are the same. It was a leftover before the refactoring. Removing duplicate.

Fixed here - CR comment - UTs (renamings and reverting SetupTest for logger) and n…

TomerShor · 2025-06-19T10:16:35Z

pkg/ingresscache/ingresscache_test.go

+	testLogger, err := nucliozap.NewNuclioZapTest("test")
+	suite.Require().NoError(err)


This IS something that can be in the SetupSuite, because the logger can be shared between tests.

Fixed here - CR comment - UTs (renamings and reverting SetupTest for logger) and n…

rokatyy

Much better now, have couple of comments and suggestions

rokatyy · 2025-06-19T10:12:19Z

pkg/ingresscache/ingresscache_test.go

+			shouldFail:     true,
+			errorMessage:   "host does not exist",


why not having just errorMessage param? If it's not empty, then shouldFail

I followed the testing convention commonly used in many Nuclio tests — for example:

https://github.com/nuclio/nuclio/blob/fb02fbd1cb176ec74f97de591a7d18224c2fe215/pkg/nuctl/client/api_test.go#L83

https://github.com/nuclio/nuclio/blob/1e4ce60a1a6d98d7c0a7d150365cf7a73cf0f4cd/pkg/processor/build/builder_test.go#L1077

I'll rename shouldFail to expectError, but aside from that, I'd prefer to keep the test structure as is.

Fixed here - CR comment - UTs (renamings and reverting SetupTest for logger) and n…

rokatyy · 2025-06-19T10:14:24Z

pkg/ingresscache/ingresscache_test.go

+		{
+			name: "Set new host",
+			args: testIngressCacheArgs{"example.com", "/test/path", "test-function-name-1"},
+			expectedResult: map[string]map[string][]string{
+				"example.com": {"/test/path": {"test-function-name-1"}},
+			},
+		}, {
+			name: "Set another functionName for existing host",
+			args: testIngressCacheArgs{"example.com", "/test/path", "test-function-name-2"},
+			initialState: []ingressCacheTestInitialState{
+				{"example.com", "/test/path", "test-function-name-1"},
+			},
+			expectedResult: map[string]map[string][]string{
+				"example.com": {"/test/path": {"test-function-name-1", "test-function-name-2"}},
+			},
+		}, {


so test case's order matters here? If somebody adds a test in between of those, it will break them?

Each test initializes its own testIngressCache based on testCase.initialState (link),
ensuring full test independence — execution order doesn't affect results.
In this specific case, the test name was a bit misleading, so I’m renaming it for clarity.

Fixed here - CR comment - UTs (renamings and reverting SetupTest for logger) and n…

rokatyy · 2025-06-19T10:16:34Z

pkg/ingresscache/safetrie.go

+
+	pathFunctionNames, ok := pathValue.([]string)
+	if !ok {
+		return errors.Errorf("value is not a []string, got: %T", pathValue)


Suggested change

return errors.Errorf("value is not a []string, got: %T", pathValue)

return errors.Errorf("path value should be []string, got %T", pathValue)

to align with the error in Delete function

Fixed here - CR comment - UTs (renamings and reverting SetupTest for logger) and n…

rokatyy · 2025-06-19T10:22:08Z

pkg/ingresscache/safetrie_test.go

+		}, {
+			name: "set different paths and different functions",
+			args: []safeTrieFunctionArgs{
+				{
+					path:     "/path/to/function",
+					function: "test-function",
+				}, {
+					path:     "/another/path/to/function/",
+					function: "test-function2",
+				},


what will happen in this case? and how it aligns with ingress behaviour in this case

Suggested change

}, {

name: "set different paths and different functions",

args: []safeTrieFunctionArgs{

{

path: "/path/to/function",

function: "test-function",

}, {

path: "/another/path/to/function/",

function: "test-function2",

},

}, {

name: "set different paths and different functions",

args: []safeTrieFunctionArgs{

{

path: "/path/to/function",

function: "test-function",

}, {

path: "/path/to/function/",

function: "test-function2",

},

There are two separate points here:

It's not possible to define two ingresses that differ only by a trailing /. For example, if there's already an ingress with the path /path/to/function, trying to add another one with /path/to/function/ will fail during the Dashboard's ingress validation.
This happens because the validation logic normalizes the URL before checking for duplicates — as seen here.

The goal of this test is to verify behavior with two distinct paths, not nested ones — so I’d prefer to keep it as is.

yeah, I didn't mean to modify the test, just was wondering how it works in this case. Then we are safe 👍

rokatyy · 2025-06-19T10:23:07Z

pkg/ingresscache/safetrie_test.go

+				{
+					path:     "///path/to/function",
+					function: "test-function",
+				},


is it something that's possible to have in our setup with ingresses?

Similar to regex — it's configurable, but not actually usable.
From the dashboard:

From the ingress yaml:

$ k -n default-tenant get ingress nuclio-test6 -oyaml | grep -A 10 rules rules: - host: my-multi-function.default-tenant.app.vmdev5.lab.iguazeng.com http: paths: - backend: service: name: nuclio-test6 port: name: http path: ///path/to/function pathType: ImplementationSpecific

But when trying to trigger this function from outside- it doesn't reach the DLX.
So even though configuring it is technically possible (despite being a bad practice and not a viable path), I'd prefer to keep the coverage in place.

rokatyy · 2025-06-19T10:24:30Z

pkg/ingresscache/safetrie_test.go

+	}
+	for _, testCase := range []struct {
+		name           string
+		arg            string


Suggested change

arg string

path string

rokatyy · 2025-06-19T10:27:10Z

pkg/ingresscache/types.go

+type IngressHostsTree interface {
+	// SetFunctionName sets a function for a given path. Will overwrite existing values if the path already exists
+	SetFunctionName(path string, function string) error
+	// DeleteFunctionName 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.
+	DeleteFunctionName(path string, function string) error
+	// GetFunctionNames retrieves the best matching function names for a given path based on longest prefix match
+	GetFunctionNames(path string) ([]string, error)
+	// IsEmpty checks if the tree is empty
+	IsEmpty() bool
+}


nitpicking - add new strings between methods for better readability

@rokatyy What do you mean new strings ?

Fixed here - CR comment - UTs (renamings and reverting SetupTest for logger) and n…

…ewlines in types.go

rokatyy

LGTM

TomerShor

Great job 👏

TomerShor requested changes Jun 11, 2025

View reviewed changes

weilerN marked this pull request as ready for review June 12, 2025 08:06

weilerN marked this pull request as draft June 12, 2025 08:07

weilerN force-pushed the nuc-420-add-ingress-cache branch 4 times, most recently from e79e841 to 08adfdb Compare June 15, 2025 11:42

weilerN added 3 commits June 15, 2025 17:09

add ingressHostsTree

dd7328f

add go.mod

34ebe66

unit tests for pathTrie

a356561

weilerN force-pushed the nuc-420-add-ingress-cache branch from b9d7f34 to a356561 Compare June 15, 2025 14:11

weilerN added 6 commits June 15, 2025 17:36

support thread safe and rename to safeTrie

59daf75

add ingressCache

530f90d

unit tests for ingressCache

e68a6d0

fix linter

11beb6d

implement IsEmpty in order ot support empty host deletion

53c4afa

IsEmpty unit tests

b4eaa16

weilerN force-pushed the nuc-420-add-ingress-cache branch from 1de4cab to b4eaa16 Compare June 16, 2025 09:53

weilerN added 3 commits June 16, 2025 15:24

Make SafeTrie delete operation more idempotent

a0488ec

CR fix-naming conventions

5100bc2

add Copyright to new files

d1741bc

weilerN force-pushed the nuc-420-add-ingress-cache branch from 1c930b0 to d1741bc Compare June 16, 2025 14:36

weilerN requested a review from TomerShor June 16, 2025 15:03

weilerN marked this pull request as ready for review June 16, 2025 15:03

TomerShor changed the title ~~Nuc 420 add ingress cache~~ [Framework] Add ingress cache Jun 16, 2025

rokatyy requested changes Jun 16, 2025

View reviewed changes

TomerShor requested changes Jun 16, 2025

View reviewed changes

weilerN added 2 commits June 17, 2025 09:37

CR comment - Copyright

5f6ef19

CR comment - rename directory and interfaces comments

e18346a

weilerN force-pushed the nuc-420-add-ingress-cache branch from b40b97f to e18346a Compare June 17, 2025 13:31

weilerN added 2 commits June 17, 2025 19:13

CR comment - improve usage of syncMap and slice

3722cc5

CR comment - improve logs and UT struct namings

d8d3fac

TomerShor requested changes Jun 18, 2025

View reviewed changes

CR comment - refactor UTs (remove mocks, consts and setupTest)

6b78f0d

weilerN requested review from TomerShor and rokatyy June 19, 2025 08:05

rokatyy reviewed Jun 19, 2025

View reviewed changes

TomerShor reviewed Jun 19, 2025

View reviewed changes

rokatyy reviewed Jun 19, 2025

View reviewed changes

CR comment - UTs (renamings and reverting SetupTest for logger) and n…

3199365

…ewlines in types.go

weilerN requested review from TomerShor and rokatyy June 19, 2025 14:02

rokatyy approved these changes Jun 19, 2025

View reviewed changes

TomerShor approved these changes Jun 19, 2025

View reviewed changes

TomerShor merged commit b73025e into v3io:development Jun 22, 2025
3 checks passed

		@@ -0,0 +1,7 @@
		package ingressCache

		type ingressHostsTree interface {

		// TODO - add log that function does not exist in the path
		return fmt.Errorf("%s, function:%s, path:%s", functionNotExistsError, function, path)

-	err := p.WalkPath(path, func(path string, value interface{}) error {
+	if err := p.WalkPath(path, func(path string, value interface{}) error {
+	    ...
+	}); err != nil {
+	    ...
+	}

	Copyright 2019 Iguazio Systems Ltd.
	Copyright 2025 Iguazio Systems Ltd.

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

	func (suite *SafeTrieTestSuite) generateLotsOfPathsAndFunctions(num int) []safeTrieFunctionArgs {
	func (suite *SafeTrieTestSuite) generatePathsAndFunctions(num int) []safeTrieFunctionArgs {

	suite.Require().Contains(err.Error(), testCase.errorMessage)
	suite.Require().ErrorContains(err, testCase.errorMessage)

		suite.Require().NotNil(err)
		suite.Require().Contains(err.Error(), testCase.errorMessage)

		testLogger, err := nucliozap.NewNuclioZapTest("test")
		suite.Require().NoError(err)

	return errors.Errorf("value is not a []string, got: %T", pathValue)
	return errors.Errorf("path value should be []string, got %T", pathValue)

Conversation

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

Uh oh!

Motivation

Description

Affected Areas

Testing

Changes Made

Additional Notes

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!

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!

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!

weilerN Jun 17, 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!

Choose a reason for hiding this comment

Uh oh!

weilerN commented Jun 11, 2025 •

edited

Loading

weilerN Jun 17, 2025 •

edited

Loading

rokatyy Jun 16, 2025 •

edited

Loading