fix: Concurrent access to trigger connection maps #2814

gokulav137 · 2023-09-23T08:34:21Z

Refactor connection maps for trigger clients from map to concurrent safe map.

Checklist:

My organization is added to USERS.md.

feat: add all sprig functions to templates (argoproj#2768)

…safe maps - HTTP client - CustomTrigger client - Kafka client - Pulsar client - NATS client - Lambda client - OpenWhisk client - Eventbus client - Servicebus client Signed-off-by: gokulav137 <gokulav999@gmail.com>

Signed-off-by: gokulav137 <gokulav999@gmail.com>

whynowy · 2023-09-25T06:23:14Z

sensors/context.go

@@ -51,25 +52,25 @@ type SensorContext struct {
 	hostname        string

 	// httpClients holds the reference to HTTP clients for HTTP triggers.
-	httpClients map[string]*http.Client


Define a generic struct in the utilities like below?

type StringKeyedMap[T any] struct { items map[string]T lock *sync.RWMutex }

Cool. I'll make the changes. Thanks.

- Add StringKeyedMap, Concurrent Safe String keyed map; using generics - Replace concurrent safe connection map structs with StringKeyedMap Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

whynowy · 2023-09-25T17:42:27Z

common/util.go

@@ -475,3 +476,35 @@ func StructToMap(obj interface{}, output map[string]interface{}) error {

 	return json.Unmarshal(data, &output) // Convert to a map
 }
+
+// Concurrent Safe String keyed map


Moving it to a dedicated file would be better, all the rest looks good to me. Thanks again!

Thanks for the quick feedback.😄Where do you think this would fit. Kinda puzzled.

I'm thinking same package but, in a concurrent.go file?

Same package, string_keyed_map.go?

I was thinking along the lines any common concurrent safe implementations required moving forward could be placed in concurrent.go. If this doesn't track, I will change the filename. Let me know.

This is even bigger...

It is good to use straightforward file names.

Oh big as in vague. concur_safe_type.go? Sorry if I'm dragging this naming out 😅.

Its just that the other files in common/ are json.go , retry.go, string.go, time.go etc are generic names

My personal opinions:

The package structure in Argo Events (including file names) should be refactored;

Those file names (json.go, string.go ...) are kind of simple, because they provide that particular file name related utility functions (not struct functions), and maybe there's no better names to describe it - you can imagine we could name them something like json_util.go or string_util.go, that wouldn't make too much difference. But it's for sure it does not make much sense to separate them into different files.

If it's util structs (like the one you added), there is already a clear boundary (the struct itself), putting them in separated files and naming them as straightforward as possible might be better, so that ppl clearly know what they are about.

This makes a lot of sense. Thanks for clarifying!

Will rename the file to string_keyed_map.go

Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

…from eventhubs to eventhub Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

whynowy · 2023-09-26T01:38:28Z

common/concurrent.go

+	lock  *sync.RWMutex
+}
+
+func NewStringKeyedMap[T any]() *StringKeyedMap[T] {


There's no need to return a pointer, right?

Makes sense. I'll make it return by value.

Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

…rgo-events into concurrent-safe-map

Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

whynowy

LGTM, thanks!

gokulav137 and others added 5 commits September 9, 2023 18:38

Merge pull request #2 from argoproj/master

ccfda47

feat: add all sprig functions to templates (argoproj#2768)

Merge branch 'argoproj:master' into master

f2de435

Fix New func for concurrent safe map creating nil maps

b78023a

Signed-off-by: gokulav137 <gokulav999@gmail.com>

Empty Commit to retrigger test

f87c2c4

Signed-off-by: gokulav137 <gokulav999@gmail.com>

gokulav137 marked this pull request as ready for review September 23, 2023 10:06

gokulav137 requested review from VaibhavPage and whynowy as code owners September 23, 2023 10:06

whynowy reviewed Sep 25, 2023

View reviewed changes

Refactor Concurrent Safe connection map structs using generics

e6e95bb

- Add StringKeyedMap, Concurrent Safe String keyed map; using generics - Replace concurrent safe connection map structs with StringKeyedMap Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

whynowy reviewed Sep 25, 2023

View reviewed changes

gokulav137 added 3 commits September 25, 2023 23:29

Move StringKeyedMap from util.go to dedicated concurret.go file

f13eb1c

Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

Segregate external imports and avoid merge conflicts

03e3998

Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

Revert unintended change - azure-event-hubs-go import alias reverted …

ed62a55

…from eventhubs to eventhub Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

gokulav137 force-pushed the concurrent-safe-map branch from 2b8f2fb to ed62a55 Compare September 25, 2023 18:16

gokulav137 added 2 commits September 26, 2023 00:21

Merge branch 'master' into concurrent-safe-map

a296f30

Merge branch 'master' into concurrent-safe-map

a011e0e

whynowy reviewed Sep 26, 2023

View reviewed changes

gokulav137 added 3 commits September 26, 2023 07:49

Refactor passing StringKeyedMap by reference to value

dd521cb

Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

Merge branch 'concurrent-safe-map' of https://github.com/gokulav137/a…

18cca0b

…rgo-events into concurrent-safe-map

Rename common/concurrent.go to common/string_keyed_map.go

c4b19b5

Signed-off-by: gokulav137 <52277763+gokulav137@users.noreply.github.com>

whynowy approved these changes Sep 27, 2023

View reviewed changes

whynowy changed the title ~~Fix: Concurrent access to trigger connection maps~~ fix: Concurrent access to trigger connection maps Sep 27, 2023

whynowy merged commit 334097f into argoproj:master Sep 27, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Concurrent access to trigger connection maps #2814

fix: Concurrent access to trigger connection maps #2814

gokulav137 commented Sep 23, 2023 •

edited

Loading

whynowy Sep 25, 2023

gokulav137 Sep 25, 2023

whynowy Sep 25, 2023

gokulav137 Sep 25, 2023 •

edited

Loading

gokulav137 Sep 25, 2023

whynowy Sep 25, 2023

gokulav137 Sep 25, 2023 •

edited

Loading

whynowy Sep 26, 2023

gokulav137 Sep 26, 2023

gokulav137 Sep 26, 2023

whynowy Sep 26, 2023

gokulav137 Sep 27, 2023 •

edited

Loading

whynowy Sep 26, 2023

gokulav137 Sep 26, 2023

whynowy left a comment

fix: Concurrent access to trigger connection maps #2814

fix: Concurrent access to trigger connection maps #2814

Conversation

gokulav137 commented Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gokulav137 Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gokulav137 Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gokulav137 Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whynowy left a comment

Choose a reason for hiding this comment

gokulav137 commented Sep 23, 2023 •

edited

Loading

gokulav137 Sep 25, 2023 •

edited

Loading

gokulav137 Sep 25, 2023 •

edited

Loading

gokulav137 Sep 27, 2023 •

edited

Loading