-
Notifications
You must be signed in to change notification settings - Fork 13
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
Feature / enable&disable duration #81
Conversation
cronsumer.go
Outdated
@@ -29,5 +29,5 @@ func New(cfg *kafka.Config, c kafka.ConsumeFn) kafka.Cronsumer { | |||
cfg.Logger.Infof("Topic [%s] verified successfully!", cfg.Consumer.Topic) | |||
} | |||
|
|||
return internal.NewCronsumer(cfg, c) | |||
return internal.NewCronsumerClient(cfg, c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, no need to put Client as suffix
internal/cron_client.go
Outdated
gocron "github.com/robfig/cron/v3" | ||
) | ||
|
||
type cronsumerClient struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, this file is not cron_client, it initializes cronsumer etc.
@@ -447,6 +447,48 @@ func Test_Should_Discard_Message_When_Header_Filter_Defined(t *testing.T) { | |||
} | |||
} | |||
|
|||
func Test_Should_Consume_Exception_Message_Successfully_When_Duration_Zero(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to refactor this
Linter check 🙏🏻 |
internal/cron_client.go
Outdated
}) | ||
} | ||
|
||
func (s *cronsumerClient) startListen(cfg kafka.ConsumerConfig) func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pay attention to goroutine leak, refactor here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some refactoring needed
By disabling the duration feature, the exception topic is continuously consumed. When the messages that come after the cron statement is triggered are consumed while they are in the currently running batch process, the current consumption process is closed and this message is published to the topic again to be consumed at the new trigger time.
This PR also includes naming changes.
#77