Feature/delete sensor #7523

guy-har · 2024-02-29T20:57:20Z

No description provided.

github-actions · 2024-02-29T21:05:01Z

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed

Thanks!

Fairly minor but important changes requested.

arielshaqed · 2024-03-05T15:23:00Z

pkg/config/config.go

@@ -287,6 +287,7 @@ type Config struct {
 	Graveler struct {
 		EnsureReadableRootNamespace bool `mapstructure:"ensure_readable_root_namespace"`
 		BatchDBIOTransactionMarkers bool `mapstructure:"batch_dbio_transaction_markers"`
+		TriggerDeleteSensorAt       int  `mapstructure:"trigger_delete_sensor_at"`


I don't know what this parameter does. Can you document it, or perhaps add units (does num_deletes_to_trigger_delete_sensor make sense?), or change the name?

arielshaqed · 2024-03-05T15:23:50Z

pkg/graveler/delete_sensor.go

+	stagingTokenID StagingToken
+}
+
+func (s *stagingTokenData) CombinedKey() string {


Please document this function. Why do we need it? Go doesn't need a string key to index a map, so if it's just for that -- you can use a struct of strings as a map key and it will work.

arielshaqed · 2024-03-05T15:26:22Z

pkg/graveler/delete_sensor.go

+}
+
+type DeleteSensor struct {
+	ctx       context.Context


Every task should probably have its own context. For instance, that allows logs to carry a request ID.

Thanks! looks like leftovers, it isn't used

arielshaqed · 2024-03-05T15:28:12Z

pkg/graveler/delete_sensor.go

+		triggerAt:              triggerAt,
+		stopped:                0,
+		graceDuration:          graceDuration,
+		mutex:                  &sync.RWMutex{},


Not sure why not

Suggested change

mutex: &sync.RWMutex{},

mutex: &sync.Mutex{},

arielshaqed · 2024-03-05T15:28:59Z

pkg/graveler/delete_sensor.go

+	// stopped used as flag that the sensor has stopped. stop processing CountDelete.
+	stopped                int32
+	graceDuration          time.Duration
+	branchTombstoneCounter map[string]*StagingTokenCounter


Please document the key.

Changed to map[RepositoryID]map[BranchID]*StagingTokenCounter. Please comment again if you still think documentation is needed

arielshaqed · 2024-03-05T15:34:10Z

pkg/graveler/delete_sensor.go

+	if stCounter.StagingTokenID != st.stagingTokenID {
+		stCounter.StagingTokenID = st.stagingTokenID
+		stCounter.Counter = 1
+		return
+	}


Why drop the count of the sealed token? Its entries will still need to be skipped.

I think that the assumption here is that if the token was sealed then a commit is in progress, meaning we will soon be rid of those entries. But then please document this decision -- we may have other reasons to seal in future.

This was done to:

Avoid the need for exposing a reset method and calling across the code (for now)

An "easy" way to reset cross lakeFS instances when a commit or compaction is done.

It isn't the best way and has many downsides, but I think it will do for the log implementation, and will give us valid information for how we would like to go on

Adding documentation

You might have a weird order of staging token for a branch, a transition of staging tokens might look like:
st1,st1,st1,st2,st1,st2,st2,st2. How will this effect the sensor logic?

I'm fine with opening an issue to refactor this and revisit at a later date.

At such a later date, I would probably try to fill out this sketch: Keep a sync.Map of branch or staging token triggers (I'm fine with either, because we don't care about losing a few counts -- in any case if we need to compact more than once a second, then we need to do something better than compaction!). Whenever we have to, atomically increment the counter on the trigger. If the count-after-increment is (exactly) 1000:

If we count per branch, atomically decrement it by 1000 and trigger compaction or whatever.

If we count per staging token, atomically replace the staging token trigger, and we're happy to lose the excess counts!

arielshaqed · 2024-03-05T15:34:50Z

pkg/graveler/delete_sensor.go

+		stCounter.Counter = 1
+		return
+	}
+	if stCounter.Counter >= s.triggerAt-1 {


Suggested change

if stCounter.Counter >= s.triggerAt-1 {

if stCounter.Counter >= s.triggerAt-1 {

This is odd. If triggerAt is 2, I would not expect the sensor to fire at the first deletion.

It won't, but I agree it makes the code unclear, did this to avoid another if.
Changed

arielshaqed · 2024-03-05T15:37:30Z

pkg/graveler/delete_sensor.go

+	if !atomic.CompareAndSwapInt32(&s.stopped, 0, 1) {
+		return
+	}
+	s.stop()


Probably worth logging if stop() timed out and returned: there may be errors during shutdown, and they will be less scary and easier to understand if Close() explains what happens.

arielshaqed · 2024-03-05T15:37:45Z

pkg/graveler/delete_sensor_test.go

+				triggredBranches[string(repositoryID)+"-"+string(branchID)]++
+			}
+			sensor := graveler.NewDeleteSensor(ctx, tc.triggerAt, cb)
+			//defer sensor.Close()


removed, it's closed later

arielshaqed · 2024-03-05T15:38:26Z

pkg/graveler/graveler.go

@@ -2382,10 +2384,21 @@ func (g *Graveler) Reset(ctx context.Context, repository *RepositoryRecord, bran
 	return nil
 }

+func (g *Graveler) deleteAndNotify(ctx context.Context, repositoryID RepositoryID, branchID BranchID, branch *Branch, key Key, requireExists bool) error {


Please add a godoc. Especially the "notify" bit :-)

arielshaqed

Moar comments about stopping.

arielshaqed · 2024-03-05T15:40:16Z

pkg/graveler/delete_sensor.go

+}
+
+func (s *DeleteSensor) CountDelete(ctx context.Context, repositoryID RepositoryID, branchID BranchID, stagingTokenID StagingToken) {
+	if s.isStopped() {


This doesn't run very often: it means that stopping the program has to wait for a delete before the sensor stops running, so Close will often wait and then time out!

arielshaqed · 2024-03-05T15:42:32Z

pkg/graveler/delete_sensor.go

+		ctx:                    ctx,
+		cb:                     cb,
+		triggerAt:              triggerAt,
+		stopped:                0,


This is racy and slow. Consider just closing thye callbacks chan to stop.

N-o-Z

Thanks,

Not blocking as to not delay you but please note the comments

N-o-Z · 2024-03-06T12:36:56Z

pkg/config/config.go

-		RepositoryCache             struct {
+		EnsureReadableRootNamespace     bool `mapstructure:"ensure_readable_root_namespace"`
+		BatchDBIOTransactionMarkers     bool `mapstructure:"batch_dbio_transaction_markers"`
+		NumDeletesToTriggerDeleteSensor int  `mapstructure:"num_deletes_to_trigger_delete_sensor"`


I don't think this is a good name :)
Consider CompactionSensorThreashold or DeleteSensorThreashold

Suggested change

NumDeletesToTriggerDeleteSensor int `mapstructure:"num_deletes_to_trigger_delete_sensor"`

CompactionSensorThreashold int `mapstructure:"compaction_sensor_threshold"`

N-o-Z · 2024-03-06T12:38:50Z

pkg/config/config.go

-		RepositoryCache             struct {
+		EnsureReadableRootNamespace     bool `mapstructure:"ensure_readable_root_namespace"`
+		BatchDBIOTransactionMarkers     bool `mapstructure:"batch_dbio_transaction_markers"`
+		NumDeletesToTriggerDeleteSensor int  `mapstructure:"num_deletes_to_trigger_delete_sensor"`


We should have a minimal value for it, otherwise we might risk damaging performance instead of improving it.
We can set it now for an arbitrary value (1000) and tune it once the compaction mechanism is in place.

I'm not sure what value we get from protecting with a minimum number. If for some reason (e.g. we add more lakeFS instances) we decide to use a smaller number, why would we want to be blocked by an arbitrary value?

N-o-Z · 2024-03-06T13:01:31Z

pkg/graveler/delete_sensor.go

+		s.branchTombstoneCounter[st.repositoryID][st.branchID] = stCounter
+		return
+	}
+	// Reset the counter if the staging token has changed, under the assumption that staging tokens are updated only during sealing processes, which occur following a commit or compaction.


Suggested change

// Reset the counter if the staging token has changed, under the assumption that staging tokens are updated only during sealing processes, which occur following a commit or compaction.

// Reset the counter if the staging token has changed, under the assumption that staging tokens are updated only during sealing processes, which occur following a failed commit or compaction.

why failed?

Sealed tokens are created only on a failed commit. On a successful commit the tokens are cleared. So either "during a commit" or "following a failed commit"

during a commit makes sense to me, changing

N-o-Z · 2024-03-06T13:02:42Z

pkg/graveler/delete_sensor.go

+
+type DeleteSensorCB func(repositoryID RepositoryID, branchID BranchID, stagingTokenID StagingToken, inGrace bool)
+
+type StagingTokenCounter struct {


Please add documentation to all the structs

N-o-Z · 2024-03-06T18:59:42Z

pkg/catalog/catalog.go

+	var deleteSensor *graveler.DeleteSensor
+	if cfg.Config.Graveler.NumDeletesToTriggerDeleteSensor > 0 {
+		cb := func(repositoryID graveler.RepositoryID, branchID graveler.BranchID, stagingTokenID graveler.StagingToken, inGrace bool) {
+			logging.FromContext(ctx).WithFields(logging.Fields{


We should consider what kind of experiment we're trying to do here. What would we like to see, how will it change our assumptions.
As part of this we should also consider the visibility of the data. Is logging enough? Do we want to add this to the statistics?
These are all questions, no concrete action requested.

Currently leaving with logs, I'm not expecting too much, and it's enough to get the information. Probably not as good as metrics, I prefer leaving it for later, and adding it if we see it's needed.

N-o-Z · 2024-03-06T19:09:34Z

pkg/graveler/delete_sensor.go

+	return atomic.LoadInt32(&s.stopped) == 1
+}
+
+func (s *DeleteSensor) triggerTombstone(ctx context.Context, st stagingTokenData) {


I think this is a potential major bottleneck.
The call to CountDelete is synchronous and using a lock for triggerTombstone could cause lowered performance on delete load.
I would expect the request to go into some channel which will handle the triggerTombstone asynchronously (or at least this block).

How can that happen? it only updates a structure, there are some cases it will update a channel, but even in that case it will not wait.

Thought about using channels at first, the two downsides were

If a channel is full, I need to throw the trigger away (or block, which is unacceptable)

It's slower with channels

This causes adding tombstones to be unavoidably serialized, since all of them have to go via this method. And if I'm not mistaken, this is true for the lakeFS server context, not just for a specific branch or repository

I fear @N-o-Z is right, which is both expected and unfortunate. I dug into AWS smithy and clients are not serialized per host, you could get multiple clients to the same DynamoDB server. So without this code deletes are not serialized.

However, this is actually not a particularly expensive piece of code, especially in all the cases where it does not enqueue an event. I suggest that we remain aware of it, profile it under load, and if necessary replace this map with a sync.Map. That has a method LoadOrStore, which would probably do a lot of what we need here. At worst we would need to get a bit sloppy with the count :-/

So I would like us to agree to open an issue to profile this and replace with sync.Map.

N-o-Z · 2024-03-06T19:15:11Z

pkg/graveler/graveler.go

@@ -1802,7 +1804,7 @@ func (g *Graveler) Delete(ctx context.Context, repository *RepositoryRecord, bra
 	log := g.log(ctx).WithFields(logging.Fields{"key": key, "operation": "delete"})
 	err = g.safeBranchWrite(ctx, log, repository, branchID,
 		safeBranchWriteOptions{}, func(branch *Branch) error {
-			return g.deleteUnsafe(ctx, repository, branch, key, nil)
+			return g.deleteUnsafe(ctx, repository, branchID, branch, key, nil)


Suggestion: Use BranchRecord instead

N-o-Z · 2024-03-06T19:17:58Z

pkg/graveler/graveler.go

 // resetKey resets given key on branch
 // Since we cannot (will not) modify sealed tokens data, we overwrite changes done on entry on a new staging token, effectively reverting it
 // to the current state in the branch committed data. If entry is not committed return an error
-func (g *Graveler) resetKey(ctx context.Context, repository *RepositoryRecord, branch *Branch, key Key, stagedValue *Value, st StagingToken) error {
+func (g *Graveler) resetKey(ctx context.Context, repository *RepositoryRecord, branchID BranchID, branch *Branch, key Key, stagedValue *Value, st StagingToken) error {


BranchRecord

itaiad200 · 2024-03-07T08:18:59Z

pkg/graveler/delete_sensor.go

+		case s.callbacks <- st:
+			stCounter.Counter = 0
+		default:
+			logging.FromContext(ctx).WithFields(logging.Fields{"repositoryID": st.repositoryID, "branchID": st.branchID, "stagingTokenID": st.stagingTokenID}).Info("delete sensor callback channel is full, dropping delete event")


I believe all related logs should be Debug

Suggested change

logging.FromContext(ctx).WithFields(logging.Fields{"repositoryID": st.repositoryID, "branchID": st.branchID, "stagingTokenID": st.stagingTokenID}).Info("delete sensor callback channel is full, dropping delete event")

logging.FromContext(ctx).WithFields(logging.Fields{"repositoryID": st.repositoryID, "branchID": st.branchID, "stagingTokenID": st.stagingTokenID}).Debug("Delete sensor callback channel is full, dropping delete event")

But we want to get this log, this is basically logging, if there was compaction, I would do it now.

I agree with @guy-har that it's an interesting log, so 👍🏽 .

But...

if the chan is full, clearly there are too many deletes -- and we start spamming logs. Not sure we can do this without some antiflooding, and I cannot find a package to do this. Let's go with Debug and open a "good first issue"; anyone can pick it up later, it should be 1-2 days' work?

itaiad200 · 2024-03-07T08:21:16Z

pkg/graveler/delete_sensor.go

+	if stCounter.StagingTokenID != st.stagingTokenID {
+		stCounter.StagingTokenID = st.stagingTokenID
+		stCounter.Counter = 1
+		return
+	}


You might have a weird order of staging token for a branch, a transition of staging tokens might look like:
st1,st1,st1,st2,st1,st2,st2,st2. How will this effect the sensor logic?

itaiad200 · 2024-03-07T08:21:38Z

pkg/graveler/delete_sensor.go

+		return
+	}
+	// Reset the counter if the staging token has changed, under the assumption that staging tokens are updated only during sealing processes, which occur following a commit or compaction.
+	if stCounter.StagingTokenID != st.stagingTokenID {


Do you want to log something before the swap?

itaiad200 · 2024-03-07T08:23:33Z

pkg/catalog/catalog.go

+				"branchID":       branchID,
+				"stagingTokenID": stagingTokenID,
+				"inGrace":        inGrace,
+			}).Info("delete sensor callback")


Suggested change

}).Info("delete sensor callback")

}).Info("Delete sensor callback")

N-o-Z

Thanks for addressing the comments. I have only one concern which I think we have to fix before this could be merged

N-o-Z · 2024-03-10T09:56:38Z

pkg/graveler/delete_sensor.go

+	return atomic.LoadInt32(&s.stopped) == 1
+}
+
+func (s *DeleteSensor) triggerTombstone(ctx context.Context, st stagingTokenData) {


This causes adding tombstones to be unavoidably serialized, since all of them have to go via this method. And if I'm not mistaken, this is true for the lakeFS server context, not just for a specific branch or repository

arielshaqed

I would like to open issues and defer on most of the remaining issues.

I do want us to use a struct as a map key.¹ This is always easier to read and more efficient IME. And it will make it more clear that we can move to sync.Map if there are performance issues.

This is non-blocking, because I accept that right now this is a matter of taste. ↩

arielshaqed · 2024-03-10T13:44:49Z

pkg/graveler/delete_sensor.go

+	callbackChannelSize = 1000
+)
+
+type DeleteSensorCB func(repositoryID RepositoryID, branchID BranchID, stagingTokenID StagingToken, inGrace bool)


Please add a godoc. It should say that e.g.

if inGrace, lakeFS is terminating and the callback should return ASAP.

Alternatively, remove this parameter entirely and just log.Info() instead of calling the callback when inGrace.

arielshaqed · 2024-03-10T13:46:46Z

pkg/graveler/delete_sensor.go

+	mutex     sync.Mutex
+	// stopped used as flag that the sensor has stopped. stop processing CountDelete.
+	stopped                bool
+	branchTombstoneCounter map[RepositoryID]map[BranchID]*StagingTokenCounter


This seems less useful than

Suggested change

branchTombstoneCounter map[RepositoryID]map[BranchID]*StagingTokenCounter

branchTombstoneCounter map[RepositoryBranch]*StagingTokenCounter

where

type RepositoryBranch struct { RepositoryID RepositoryID BranchID BranchID }

arielshaqed · 2024-03-10T15:02:12Z

pkg/graveler/delete_sensor.go

+	return atomic.LoadInt32(&s.stopped) == 1
+}
+
+func (s *DeleteSensor) triggerTombstone(ctx context.Context, st stagingTokenData) {


I fear @N-o-Z is right, which is both expected and unfortunate. I dug into AWS smithy and clients are not serialized per host, you could get multiple clients to the same DynamoDB server. So without this code deletes are not serialized.

However, this is actually not a particularly expensive piece of code, especially in all the cases where it does not enqueue an event. I suggest that we remain aware of it, profile it under load, and if necessary replace this map with a sync.Map. That has a method LoadOrStore, which would probably do a lot of what we need here. At worst we would need to get a bit sloppy with the count :-/

So I would like us to agree to open an issue to profile this and replace with sync.Map.

arielshaqed · 2024-03-10T15:07:34Z

pkg/graveler/delete_sensor.go

+	if stCounter.StagingTokenID != st.stagingTokenID {
+		stCounter.StagingTokenID = st.stagingTokenID
+		stCounter.Counter = 1
+		return
+	}


I'm fine with opening an issue to refactor this and revisit at a later date.

At such a later date, I would probably try to fill out this sketch: Keep a sync.Map of branch or staging token triggers (I'm fine with either, because we don't care about losing a few counts -- in any case if we need to compact more than once a second, then we need to do something better than compaction!). Whenever we have to, atomically increment the counter on the trigger. If the count-after-increment is (exactly) 1000:

If we count per branch, atomically decrement it by 1000 and trigger compaction or whatever.

If we count per staging token, atomically replace the staging token trigger, and we're happy to lose the excess counts!

arielshaqed · 2024-03-10T15:27:22Z

pkg/graveler/delete_sensor.go

+		case s.callbacks <- st:
+			stCounter.Counter = 0
+		default:
+			logging.FromContext(ctx).WithFields(logging.Fields{"repositoryID": st.repositoryID, "branchID": st.branchID, "stagingTokenID": st.stagingTokenID}).Info("delete sensor callback channel is full, dropping delete event")


I agree with @guy-har that it's an interesting log, so 👍🏽 .

But...

if the chan is full, clearly there are too many deletes -- and we start spamming logs. Not sure we can do this without some antiflooding, and I cannot find a package to do this. Let's go with Debug and open a "good first issue"; anyone can pick it up later, it should be 1-2 days' work?

github-actions · 2024-03-11T14:09:54Z

♻️ PR Preview b8ceda5 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

guy-har · 2024-03-11T14:25:55Z

@N-o-Z @arielshaqed , I am adding here some information
I've added a lakectl abuse command for random deletes (doesn't have to be random, but I don't think it matters that much)

I ran it:

on a local lakeFS instance configured with local
on a repo with 5000 objects
100 go routines
delete 4000 objects

running with and without the sensor, resulted the same

completed: 4000, errors: 0, current rate: 14324.46 done/second

Histogram (ms):
1	0
2	0
5	369
7	3817
10	3900
15	4000
25	4000
50	4000
75	4000
100	4000
250	4000
350	4000
500	4000
750	4000
1000	4000
5000	4000
min	4
max	15
total	4000

Pushed it a bit by setting the callback channel to 1 and setting the callback with a 1 second sleep

completed: 4000, errors: 0, current rate: 13858.10 done/second
Histogram (ms):  
1 0  
2 0  
5 141  
7 3480  
10 3913  
15 4000  
25 4000  
50 4000  
75 4000  
100 4000  
250 4000  
350 4000  
500 4000  
750 4000  
1000 4000  
5000 4000  
min 5  
max 15  
total 4000

Same as above but without the log when the channel is full (looks the same as the first)

completed: 4000, errors: 0, current rate: 14355.29 done/second
Histogram (ms):  
1 0  
2 0  
5 289  
7 3850  
10 3923  
15 4000  
25 4000  
50 4000  
75 4000  
100 4000  
250 4000  
350 4000  
500 4000  
750 4000  
1000 4000  
5000 4000  
min 4  
max 13  
total 4000

arielshaqed · 2024-03-11T14:27:23Z

@N-o-Z @arielshaqed , I am adding here some information I've added a lakectl abuse command for random deletes (doesn't have to be random, but I don't think it matters that much)

Thanks, I think that there is no difference between any of the versions that you showed.

N-o-Z

I'm convinced 😃
Thanks!!

guy-har added 4 commits February 25, 2024 14:41

Initial simple implementation

272bb8d

implement delete sensor

a1f8d1b

Use commit sensor in graveler with logging callback

b5d14c8

Remove unused error

4e978da

Remove timeout test

b6607fb

guy-har linked an issue Mar 5, 2024 that may be closed by this pull request

Add a Compaction Sensor #7497

Closed

guy-har added the exclude-changelog PR description should not be included in next release changelog label Mar 5, 2024

guy-har marked this pull request as ready for review March 5, 2024 10:40

guy-har requested review from N-o-Z and arielshaqed March 5, 2024 10:40

arielshaqed requested changes Mar 5, 2024

View reviewed changes

arielshaqed reviewed Mar 5, 2024

View reviewed changes

Code review changes

7787230

N-o-Z reviewed Mar 6, 2024

View reviewed changes

itaiad200 reviewed Mar 7, 2024

View reviewed changes

guy-har added 2 commits March 10, 2024 10:58

Code review changes

d5c5a4c

Remove grace duration and use boolean stopped instead of atomic

d94c8a3

guy-har requested review from N-o-Z and arielshaqed March 10, 2024 09:47

Validator fix

b859285

N-o-Z requested changes Mar 10, 2024

View reviewed changes

change documentation

63fcef9

arielshaqed approved these changes Mar 10, 2024

View reviewed changes

guy-har added 2 commits March 11, 2024 15:47

Add random delete abuse

72e8037

Add abuse random delete documentation

b8ceda5

N-o-Z approved these changes Mar 11, 2024

View reviewed changes

guy-har merged commit 1706a19 into master Mar 12, 2024
36 checks passed

guy-har deleted the feature/delete-sensor branch March 12, 2024 09:30

	if stCounter.Counter >= s.triggerAt-1 {
	if stCounter.Counter >= s.triggerAt-1 {

	NumDeletesToTriggerDeleteSensor int `mapstructure:"num_deletes_to_trigger_delete_sensor"`
	CompactionSensorThreashold int `mapstructure:"compaction_sensor_threshold"`

	// Reset the counter if the staging token has changed, under the assumption that staging tokens are updated only during sealing processes, which occur following a commit or compaction.
	// Reset the counter if the staging token has changed, under the assumption that staging tokens are updated only during sealing processes, which occur following a failed commit or compaction.


		type DeleteSensorCB func(repositoryID RepositoryID, branchID BranchID, stagingTokenID StagingToken, inGrace bool)

		type StagingTokenCounter struct {

	logging.FromContext(ctx).WithFields(logging.Fields{"repositoryID": st.repositoryID, "branchID": st.branchID, "stagingTokenID": st.stagingTokenID}).Info("delete sensor callback channel is full, dropping delete event")
	logging.FromContext(ctx).WithFields(logging.Fields{"repositoryID": st.repositoryID, "branchID": st.branchID, "stagingTokenID": st.stagingTokenID}).Debug("Delete sensor callback channel is full, dropping delete event")

	}).Info("delete sensor callback")
	}).Info("Delete sensor callback")

	branchTombstoneCounter map[RepositoryID]map[BranchID]*StagingTokenCounter
	branchTombstoneCounter map[RepositoryBranch]*StagingTokenCounter

Feature/delete sensor #7523

Feature/delete sensor #7523

Conversation

guy-har commented Feb 29, 2024

github-actions bot commented Feb 29, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed left a 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

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

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

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

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z left a 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

But...

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Footnotes

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

But...

github-actions bot commented Mar 11, 2024 • edited Loading

guy-har commented Mar 11, 2024

arielshaqed commented Mar 11, 2024

N-o-Z left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 11, 2024 •

edited

Loading