pbacsko commented on code in PR #581:
URL: https://github.com/apache/yunikorn-core/pull/581#discussion_r1256184242
##########
pkg/scheduler/objects/application_test.go:
##########
@@ -1990,17 +1980,16 @@ func TestAskEvents(t *testing.T) {
noEvents := 0
err = common.WaitFor(10*time.Millisecond, time.Second, func() bool {
noEvents = eventSystem.Store.CountStoredEvents()
- return noEvents == 2
+ return noEvents == 5
})
- assert.NilError(t, err, "expected 2 events, got %d", noEvents)
+ assert.NilError(t, err, "expected 5 events, got %d", noEvents)
records := eventSystem.Store.CollectEvents()
- assert.Equal(t, 2, len(records), "number of events")
- assert.Equal(t, si.EventRecord_APP, records[0].Type)
- assert.Equal(t, si.EventRecord_ADD, records[0].EventChangeType)
- assert.Equal(t, si.EventRecord_APP_REQUEST,
records[0].EventChangeDetail)
- assert.Equal(t, si.EventRecord_APP, records[1].Type)
- assert.Equal(t, si.EventRecord_REMOVE, records[1].EventChangeType)
- assert.Equal(t, si.EventRecord_REQUEST_CANCEL,
records[1].EventChangeDetail)
+ assert.Equal(t, 5, len(records), "number of events")
+ isNewApplicationEvent(t, app, records[0])
+ isStateChangeEvent(t, app, si.EventRecord_APP_ACCEPTED, records[1])
+ isNewAllocAskEvent(t, ask, records[2])
+ isAllocCancelEvent(t, ask, records[3])
+ isStateChangeEvent(t, app, si.EventRecord_APP_COMPLETING, records[4])
Review Comment:
All these new changes should not be needed... Too much noise. These are
tested in a new test (which I suggested above).
There are multiple things we can do, but I believe the simplest is to use a
function as variables so that can be replaced with nop:
```
func (sa *Application) HandleApplicationEvent(event applicationEvent) error {
sa.handleAppEventFn(event)
}
// HandleApplicationEventWithInfo handles the state event for the
application with associated info object.
// The application lock is expected to be held.
func (sa *Application) HandleApplicationEventWithInfo(event
applicationEvent, eventInfo string) error {
sa.handleAppEventWithInfoFn(event)
}
```
In both cases, the functions are variables inside `Application`. By default,
they point to the current implementation:
```
type Application struct {
...
rmEventHandler handler.EventHandler
rmID string
terminatedCallback func(appID string)
appEvents *applicationEvents
handleAppEventFn func(applicationEvent)
handleAppEventWithInfoFn func(applicationEvent,string)
sync.RWMutex
}
func NewApplication(siApp *si.AddApplicationRequest, ugi security.UserGroup,
eventHandler handler.EventHandler, rmID string) *Application {
...
app.handleAppEventFn = app.handleAppEvent
app.handleAppEventWithInfoFn = app.handleAppEventWithInfoFn
...
}
```
The new methods `handleAppEvent()` and `handleAppEventWithInfoFn()` are the
same as their exported counterpart, so you move the current implementation to
these new methods which start with a lowercase.
In the tests, you simply replace these with dummy nop functions so no
transition events will be generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
