Graylog2 / Graylog2/graylog-plugin-integrations
AWS Kinesis Code Improvement
- Dominant language
- Java
- Stars
- 17
- Forks
- 14
- PR merge metrics
- No merged PRs in 30d
Description
# Description
@alex-konn Did a review of the AWS Kinesis code, and provided some great suggestions for improvement of the code. I am capturing these in an issue so they can be addressed outside of PR #279.
## Metrics
- Maybe it makes sense to provide a predefined dashboard with graylog's
own kinesis metrics to see, how far "our" workers are behind with
processing the stream
- https://docs.aws.amazon.com/streams/latest/dev/monitoring-with-kcl.html
- Unit tests would be helpful, would require mocking out the assume role
call
## Comments
- I would get rid of most of the comments as the code is pretty concise
and clear, no need to restate what it does
- Keep comments explaining WHY some things are done though, as the assume
role stuff is not very intuitive
- In general i find that most comments do more harm than good by
bloating the amount of text one has to grok to understand what a class
does and falling out of sync with the code when people change code but
forget to update comments
- [ ] Remove comments https://github.com/Graylog2/graylog-plugin-integrations/pull/409
## AWSRequestImpl
- [ ] Get rid of `AWSRequest` interface and just use this as base class
=> no repeated field definitions while retaining type safety
## CloudWatchService
- I wouldn't use javax exceptions like `BadRequestException` here. it
couples the services to jersey and being used in a http service.
but changing that might be out of scope for this pr. also i've seen
this in many places in graylog-server already. so it might be something
like a convention
- [ ] Remove `BadRequestException` in CloudWatch Service
- the "correct" way for me would be throw a custom `RuntimeException`
and either catch and throw `BadRequestException` in the resource or use
a jersey exception mapper to map the exception to an http response
- [ ] Improve how exceptions are thrown in CloudWatch Service
- Another general comment: i find such long methods hard to understand.
it helps to extract short private methods with meaningful names.
that's also a very useful technique to discover parts of related code
that can later be extracted to their own classes with a single
responsibility.
e.g., `addSubscriptionFilter`
- [ ] extract short private methods with meaningful names
## KinesisServiceTest
- Needs guice setup for ignored test to run (and fail)
GuiceInjectorHolder.createInjector(Collections.emptyList());
- [ ] Include Guice set up for unit tests
Contributor guide
Assessment
This issue has not been assessed yet.