name kafka streams topology nodes appropriately#79
Conversation
This comment has been minimized.
This comment has been minimized.
| inputTopics.addAll(viewGenConfig.getStringList(INPUT_TOPICS_CONFIG_KEY)); | ||
| } | ||
| return inputTopics.stream().collect(Collectors.toList()); | ||
| return new ArrayList<>(inputTopics); |
There was a problem hiding this comment.
nit - why this change? List.copyOf (unless we're intentionally making this mutable)
That said, we should make this deterministic and avoid the hash set here.
(same comment below)
There was a problem hiding this comment.
why this change?
Intellij suggestion
avoid the hash set here
set needed as input topic across all viewgens is same and we need to add only one source per input topic.
There was a problem hiding this comment.
set needed as input topic across all viewgens is same and we need to add only one source per input topic.
So we have a uniqueness requirement, not a set it sounds like. Using a hash set means that different runs with the same input will return their outputs in a different order. Even if that's not impactful today, it's a bad practice and makes writing tests annoying. Suggest any of
- use a list, than run distinct on it
- use an insertion-ordered set (e.g. guava's or linkedhashset I believe)
- use streams e.g.
return getViewGenName(properties)
.stream()
.map(viewGenConfigs::get)
.map(config -> config.getStringList(INPUT_TOPICS_CONFIG_KEY))
.flatMap(Collection::stream)
.distinct()
.collect(Collectors.toUnmodifiableList());There was a problem hiding this comment.
Why not a TreeSet?
Set for me ensures uniqueness and TreeSet ensures sorted ordering (for tests thingy you mentioned).
Looks simpler and much readable than the streams snippet in the suggestion.
There was a problem hiding this comment.
Looks simpler and much readable than the streams snippet in the suggestion.
matter of opinion! but yes, any ordered set will do for repeatability, doesn't need to be insertion ordered - I was just trying to match the perceived intent.
laxmanchekka
requested review from
kishansairam9,
kotharironak and
saxenakshitiz
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #79 +/- ##
============================================
+ Coverage 75.50% 75.54% +0.03%
Complexity 148 148
============================================
Files 15 15
Lines 641 642 +1
Branches 64 64
============================================
+ Hits 484 485 +1
Misses 118 118
Partials 39 39
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This comment has been minimized.
This comment has been minimized.
|
fixed the comments and vulnerabilities. |
| implementation("com.typesafe:config:1.4.1") | ||
| compileOnly("org.projectlombok:lombok:1.18.26") | ||
| annotationProcessor("org.projectlombok:lombok:1.18.26") | ||
| implementation("org.slf4j:slf4j-api:2.0.5") |
There was a problem hiding this comment.
After this change, hypertrace/hypertrace-ingestor transitively loads slf4j-api:2.0.5 via dependency on hypertrace/view-generator-framework, but internally it still uses log4j-slf4j-impl instead of slf4j2. That led to failure of integration tests in QS which loads container "hypertrace/hypertrace-view-generator:main".
There was a problem hiding this comment.
Raised PR to fix it: hypertrace/hypertrace-ingester#425
3 participants
No description provided.