You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/08/06 21:14:24 UTC
[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #294: YUNIKORN-786 refactor TestHistoricalPartitionInfoUpdater to make it n…
yangwwei commented on a change in pull request #294:
URL: https://github.com/apache/incubator-yunikorn-core/pull/294#discussion_r684505600
##########
File path: pkg/metrics/metrics_collector_test.go
##########
@@ -27,32 +27,66 @@ import (
"github.com/apache/incubator-yunikorn-core/pkg/metrics/history"
)
-func TestHistoricalPartitionInfoUpdater(t *testing.T) {
+func TestStop(t *testing.T) {
metricsHistory := history.NewInternalMetricsHistory(3)
- setInternalMetricsCollectorTicker(5 * time.Millisecond)
- metricsCollector := NewInternalMetricsCollector(metricsHistory)
+ metricsCollector := newInternalMetricsCollector(metricsHistory, 1*time.Second)
metricsCollector.StartService()
- go func() {
- metrics := GetSchedulerMetrics()
- for i := 0; i < 3; i++ {
- metrics.IncTotalApplicationsRunning()
- metrics.AddAllocatedContainers(2)
- time.Sleep(4 * time.Millisecond)
- }
- }()
+ metricsCollector.Stop()
+ // wait for the thread to store record. it should not happen
+ time.Sleep(1500 * time.Millisecond)
+
+ records := metricsHistory.GetRecords()
+ assert.Equal(t, 3, len(records), "Expected exactly 3 history records")
+ for _, record := range records {
+ assert.Assert(t, record == nil, "The 1st item should be nil!")
+ }
+}
- time.Sleep(11 * time.Millisecond)
+func TestStartService(t *testing.T) {
+ metricsHistory := history.NewInternalMetricsHistory(3)
+ metricsCollector := newInternalMetricsCollector(metricsHistory, 1*time.Second)
+ metricsCollector.StartService()
+
+ // wait for the thread to store record
+ time.Sleep(1500 * time.Millisecond)
metricsCollector.Stop()
+ records := metricsHistory.GetRecords()
+ assert.Equal(t, 3, len(records), "Expected exactly 3 history records")
+ for i, record := range records {
+ if i == 2 {
+ assert.Assert(t, record != nil, "The 1st item should NOT be nil!")
+ } else {
+ assert.Assert(t, record == nil, "All items should be nil!")
+ }
+ }
+}
+
+func TestHistoricalPartitionInfoUpdater(t *testing.T) {
+ metricsHistory := history.NewInternalMetricsHistory(3)
+ metricsCollector := NewInternalMetricsCollector(metricsHistory)
+
+ metrics := GetSchedulerMetrics()
+
+ // skip to store record for first application
+ metrics.IncTotalApplicationsRunning()
+ metrics.AddAllocatedContainers(2)
+
+ metrics.IncTotalApplicationsRunning()
+ metrics.AddAllocatedContainers(2)
+ metricsCollector.store()
+
+ metrics.IncTotalApplicationsRunning()
+ metrics.AddAllocatedContainers(2)
+ metricsCollector.store()
+
records := metricsHistory.GetRecords()
assert.Equal(t, 3, len(records), "Expected exactly 3 history records")
for i, record := range records {
switch i {
case 0:
- if record != nil {
- t.Fatal("The 1st item should be nil!")
- }
+ assert.Assert(t, record == nil, "The 1st item should be nil!")
Review comment:
similar to the earlier comment:
```
fmt.Sprintf("record should be nil, index: %d", i)
```
##########
File path: pkg/metrics/metrics_collector_test.go
##########
@@ -27,32 +27,66 @@ import (
"github.com/apache/incubator-yunikorn-core/pkg/metrics/history"
)
-func TestHistoricalPartitionInfoUpdater(t *testing.T) {
+func TestStop(t *testing.T) {
metricsHistory := history.NewInternalMetricsHistory(3)
- setInternalMetricsCollectorTicker(5 * time.Millisecond)
- metricsCollector := NewInternalMetricsCollector(metricsHistory)
+ metricsCollector := newInternalMetricsCollector(metricsHistory, 1*time.Second)
metricsCollector.StartService()
- go func() {
- metrics := GetSchedulerMetrics()
- for i := 0; i < 3; i++ {
- metrics.IncTotalApplicationsRunning()
- metrics.AddAllocatedContainers(2)
- time.Sleep(4 * time.Millisecond)
- }
- }()
+ metricsCollector.Stop()
+ // wait for the thread to store record. it should not happen
+ time.Sleep(1500 * time.Millisecond)
+
+ records := metricsHistory.GetRecords()
+ assert.Equal(t, 3, len(records), "Expected exactly 3 history records")
+ for _, record := range records {
+ assert.Assert(t, record == nil, "The 1st item should be nil!")
+ }
+}
- time.Sleep(11 * time.Millisecond)
+func TestStartService(t *testing.T) {
+ metricsHistory := history.NewInternalMetricsHistory(3)
+ metricsCollector := newInternalMetricsCollector(metricsHistory, 1*time.Second)
+ metricsCollector.StartService()
+
+ // wait for the thread to store record
+ time.Sleep(1500 * time.Millisecond)
metricsCollector.Stop()
+ records := metricsHistory.GetRecords()
+ assert.Equal(t, 3, len(records), "Expected exactly 3 history records")
+ for i, record := range records {
+ if i == 2 {
+ assert.Assert(t, record != nil, "The 1st item should NOT be nil!")
+ } else {
+ assert.Assert(t, record == nil, "All items should be nil!")
+ }
+ }
+}
+
+func TestHistoricalPartitionInfoUpdater(t *testing.T) {
+ metricsHistory := history.NewInternalMetricsHistory(3)
+ metricsCollector := NewInternalMetricsCollector(metricsHistory)
+
+ metrics := GetSchedulerMetrics()
+
+ // skip to store record for first application
+ metrics.IncTotalApplicationsRunning()
+ metrics.AddAllocatedContainers(2)
+
+ metrics.IncTotalApplicationsRunning()
+ metrics.AddAllocatedContainers(2)
+ metricsCollector.store()
+
+ metrics.IncTotalApplicationsRunning()
+ metrics.AddAllocatedContainers(2)
+ metricsCollector.store()
+
records := metricsHistory.GetRecords()
assert.Equal(t, 3, len(records), "Expected exactly 3 history records")
for i, record := range records {
switch i {
case 0:
- if record != nil {
- t.Fatal("The 1st item should be nil!")
- }
+ assert.Assert(t, record == nil, "The 1st item should be nil!")
case 1:
assert.Equal(t, 2, record.TotalApplications, "Expected exactly 2 applications at 10 msec")
Review comment:
you may want to remove the word: `at 10msec` in these messages since now it is not depending on that.
and for this one (not introduced by this PR but still would be good to get it fixed)
```
case 2:
assert.Equal(t, 3, record.TotalApplications, "Expected exactly 3 applications at 20 msec")
assert.Equal(t, 6, record.TotalContainers, "Expected exactly 4 allocations at 20 msec")
}
```
the message for the last assert should be "expected exactly 6 allocations"
##########
File path: pkg/metrics/metrics_collector_test.go
##########
@@ -27,32 +27,66 @@ import (
"github.com/apache/incubator-yunikorn-core/pkg/metrics/history"
)
-func TestHistoricalPartitionInfoUpdater(t *testing.T) {
+func TestStop(t *testing.T) {
metricsHistory := history.NewInternalMetricsHistory(3)
- setInternalMetricsCollectorTicker(5 * time.Millisecond)
- metricsCollector := NewInternalMetricsCollector(metricsHistory)
+ metricsCollector := newInternalMetricsCollector(metricsHistory, 1*time.Second)
metricsCollector.StartService()
- go func() {
- metrics := GetSchedulerMetrics()
- for i := 0; i < 3; i++ {
- metrics.IncTotalApplicationsRunning()
- metrics.AddAllocatedContainers(2)
- time.Sleep(4 * time.Millisecond)
- }
- }()
+ metricsCollector.Stop()
+ // wait for the thread to store record. it should not happen
+ time.Sleep(1500 * time.Millisecond)
+
+ records := metricsHistory.GetRecords()
+ assert.Equal(t, 3, len(records), "Expected exactly 3 history records")
+ for _, record := range records {
+ assert.Assert(t, record == nil, "The 1st item should be nil!")
+ }
+}
- time.Sleep(11 * time.Millisecond)
+func TestStartService(t *testing.T) {
+ metricsHistory := history.NewInternalMetricsHistory(3)
+ metricsCollector := newInternalMetricsCollector(metricsHistory, 1*time.Second)
+ metricsCollector.StartService()
+
+ // wait for the thread to store record
+ time.Sleep(1500 * time.Millisecond)
metricsCollector.Stop()
+ records := metricsHistory.GetRecords()
+ assert.Equal(t, 3, len(records), "Expected exactly 3 history records")
+ for i, record := range records {
+ if i == 2 {
+ assert.Assert(t, record != nil, "The 1st item should NOT be nil!")
+ } else {
+ assert.Assert(t, record == nil, "All items should be nil!")
Review comment:
we may want to slight change the message:
```
if i == 2 {
assert.Assert(t, record != nil, fmt.Sprintf("record should not be nil, index: %d", i))
} else {
assert.Assert(t, record == nil, fmt.Sprintf("record should be nil, index: %d", i))
}
```
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org