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

grouped ns lookup containers #441

Merged

Conversation

Mortom123
Copy link
Contributor

See #438 / #438 (comment) 1.PR

Motivation

rename init container resources to better reflect its actual use (nslookup)

Modifications

rename init container resources

Verifying this change

@lhotari
Copy link
Member

lhotari commented Jan 24, 2024

rename init container resources to better reflect its actual use (nslookup)

Perhaps we could come up with a better name? nslookup is the command that happens to be used. It would be better to have a name that matches what is being done. The old names reflected this, but we could abstract it to some level, let's say that it's about waiting for a DNS name to become available.
waitDnsNameAvailable (following Helm naming conventions)?

@Mortom123
Copy link
Contributor Author

I agree. I also thought about unifying all init container resources. But some start JVM processes, afaik. So this might not be a good approach. What do you think?

@lhotari
Copy link
Member

lhotari commented Jan 24, 2024

I agree. I also thought about unifying all init container resources. But some start JVM processes, afaik. So this might not be a good approach. What do you think?

I guess that in most cases there isn't a need to tune the init container resources and it's not a concern for most users. A configuration structure with a common setting could be simpler to adjust?
Keeping things simple should be a guideline also here. Since init containers are short living, there's not necessarily a need to have minimal resources whether it's an init container that uses a JVM or not. Having sufficient resources for all use cases would be needed if there's a single shared config. If we go down the path of a single setting, let's do that.
I don't think that we should have non-JVM or JVM init container settings. Let's keep the current solution instead of doing that.

@Mortom123 Mortom123 force-pushed the feature/initResource_rename_nslookup branch from 3ebcd7e to d387fdc Compare January 24, 2024 21:39
@Mortom123
Copy link
Contributor Author

Mortom123 commented Jan 24, 2024

@Mortom123 Mortom123 force-pushed the feature/initResource_rename_nslookup branch from d387fdc to 0951f18 Compare January 24, 2024 21:42
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari merged commit 4daf6d8 into apache:master Jan 26, 2024
27 checks passed
lhotari added a commit to lhotari/pulsar-helm-chart that referenced this pull request Jan 29, 2024
@Mortom123 Mortom123 deleted the feature/initResource_rename_nslookup branch January 30, 2024 08:07
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.

None yet

2 participants