-
Notifications
You must be signed in to change notification settings - Fork 80
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
Adding support for M1 #35
Conversation
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.
Thanks for the PR @oleksandrivan - we just need to revert the changes indicated below.
implementation('io.confluent:kafka-streams-avro-serde:6.1.1') { | ||
exclude group: 'org.apache.kafka', module: 'kafka-clients' | ||
exclude group: 'org.apache.kafka', module: 'kafka-streams' | ||
} | ||
|
||
testImplementation "org.apache.kafka:kafka-streams-test-utils:2.8.0" | ||
testImplementation "org.apache.kafka:kafka-streams-test-utils:3.3.0" | ||
testImplementation "junit:junit:4.13.2" |
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.
Might as well go with 3.5.1
for all of these since that's currently the latest version
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.
Updated to 3.5.1
hostname: broker | ||
container_name: broker | ||
ports: | ||
- 29092:29092 |
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.
- 29092:29092 | |
- 9092:9092 |
This way, users can use the more familiar 9092 port for connecting to Kafka
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.
For this change I'll create another PR with the appropiate documentation so it can be ran locally.
environment: | ||
KAFKA_BROKER_ID: 1 | ||
KAFKA_LISTENER_SECURITY_PROTOCOL_MAP: PLAINTEXT:PLAINTEXT,PLAINTEXT_HOST:PLAINTEXT,CONTROLLER:PLAINTEXT | ||
KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://broker:9092,PLAINTEXT_HOST://localhost:29092 |
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.
KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://broker:9092,PLAINTEXT_HOST://localhost:29092 | |
KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://broker:29092,PLAINTEXT_HOST://localhost:9092 |
This way, users can use the more familiar 9092
port for connecting to Kafka
KAFKA_PROCESS_ROLES: broker,controller | ||
KAFKA_NODE_ID: 1 | ||
KAFKA_CONTROLLER_QUORUM_VOTERS: 1@broker:29093 | ||
KAFKA_LISTENERS: PLAINTEXT://broker:9092,CONTROLLER://broker:29093,PLAINTEXT_HOST://0.0.0.0:29092 |
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.
KAFKA_LISTENERS: PLAINTEXT://broker:9092,CONTROLLER://broker:29093,PLAINTEXT_HOST://0.0.0.0:29092 | |
KAFKA_LISTENERS: PLAINTEXT://broker:29092,CONTROLLER://broker:29093,PLAINTEXT_HOST://0.0.0.0:9092 |
- 8081:8081 | ||
environment: | ||
SCHEMA_REGISTRY_HOST_NAME: schema-registry | ||
SCHEMA_REGISTRY_KAFKASTORE_BOOTSTRAP_SERVERS: broker:9092 |
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.
SCHEMA_REGISTRY_KAFKASTORE_BOOTSTRAP_SERVERS: broker:9092 | |
SCHEMA_REGISTRY_KAFKASTORE_BOOTSTRAP_SERVERS: broker:29092 |
@@ -39,7 +39,12 @@ public static void main(String[] args) throws IOException { | |||
|
|||
// Now take the electronicStream object, group by key and perform an aggregation | |||
// Don't forget to convert the KTable returned by the aggregate call back to a KStream using the toStream method | |||
electronicStream.groupByKey().aggregate(null, null); |
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.
These tutorials were intentionally left incomplete, so users can finish the code on their own. Full solutions are found in the solution
package for each tutorial.
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.
Yep sorry, I pushed to my main branch after completing the course and the PR auto-updated to the latest commit.
// This return null statement is here so the code will compile | ||
// You need to replace it with some logic described below | ||
return null; | ||
return DeserializationHandlerResponse.CONTINUE; |
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.
same as above
if(exception instanceof RecordTooLargeException){ | ||
return ProductionExceptionHandlerResponse.CONTINUE; | ||
} | ||
return ProductionExceptionHandlerResponse.FAIL; | ||
// If the exception type is RecordTooLargeException continue working |
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.
Same as above - there are a bunch of other changes below that follow the same pattern, but I won't repeat the comments.
@@ -1,5 +1,5 @@ | |||
distributionBase=GRADLE_USER_HOME | |||
distributionPath=wrapper/dists | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.0-bin.zip | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.3-bin.zip |
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.
Bumped gradle so it uses Java 17 as Java 16 is non-LTS and no longer has support.
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.
Auto-generated by ./gradlew wrapper --gradle-version=7.3
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.
LGTM! Thanks for the contribution @oleksandrivan !
Hello.
As in this PR there is an upgrade to RocksDB library to support M1 ARM architecture, this changes were not propagated to this repository causing it to fail when trying the solution of the KTables exercise.
With this PR the streams library is updated so now it correctly works.