Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: new Mapper example which serves as a slow container for testing purposes #172

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

juliev0
Copy link
Contributor

@juliev0 juliev0 commented Jan 16, 2025

For Numaplane, it's useful to be able to simulate a slow Vertex. This does a single "cat" but does a sleep first.

Sleep time is configurable by environment variable.

}

func (e *CatSleep) Map(ctx context.Context, keys []string, d mapper.Datum) mapper.Messages {
time.Sleep(10 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make the duration configurable? thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juliev0 do you plan to constantly use this image to run e2e tests? If so, numaflow-go supports auto-updating the image such that the quay.io/numaio/numaflow-go/map-cat-sleep:stable image is always up-to-date(using the numaflow-go main). If you want this image to be auto-updated every time we change numaflow-go SDK, please add it in the auto-build list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks! I will do that. I was actually wondering if I'd need to go in here all the time and update that.

Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
@juliev0 juliev0 marked this pull request as ready for review January 16, 2025 22:59
Copy link
Member

@vigith vigith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT

func (e *CatSleep) Map(ctx context.Context, keys []string, d mapper.Datum) mapper.Messages {

// sleep for as long as the environment variable indicates (or default if not configured)
sleepSeconds := DEFAULT_SLEEP_SECONDS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could move this inside CatSleep?

Comment on lines 22 to 32
secondsString := os.Getenv("SLEEP_SECONDS")
if secondsString == "" {
log.Printf("SLEEP_SECONDS environment variable not set, using default %d seconds\n", DEFAULT_SLEEP_SECONDS)
} else {
val, err := strconv.Atoi(secondsString)
if err != nil {
log.Printf("SLEEP_SECONDS environment variable %q not an int, using default %d seconds\n", secondsString, DEFAULT_SLEEP_SECONDS)
} else {
sleepSeconds = val
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be done in main?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I suppose I can make this purposely slow container more efficient. :D

but it would still be neater though, I agree

sleepSeconds = val
}
}
time.Sleep(time.Duration(sleepSeconds) * time.Second)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
time.Sleep(time.Duration(sleepSeconds) * time.Second)
time.Sleep(time.Duration(e.sleepSeconds) * time.Second)

@juliev0
Copy link
Contributor Author

juliev0 commented Jan 17, 2025

Decided to rename to 'slow cat' and also relocated the logic to reduce code redundancy (and reduce log spew, which would've been pretty ugly too) (thanks for the suggestion @vigith)

@juliev0
Copy link
Contributor Author

juliev0 commented Jan 17, 2025

By the way, I've tested the image out in my pipeline, both with and without environment variable specified.

@KeranYang KeranYang merged commit 2b9e9e9 into main Jan 17, 2025
3 checks passed
@KeranYang KeranYang deleted the cat_sleep branch January 17, 2025 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants