Skip to content

Kamir patch 2#468

Merged
novatechflow merged 21 commits into
mainfrom
kamir-patch-2
Sep 12, 2024
Merged

Kamir patch 2#468
novatechflow merged 21 commits into
mainfrom
kamir-patch-2

Conversation

@kamir
Copy link
Copy Markdown
Contributor

@kamir kamir commented Sep 4, 2024

I have added the word_count.sh and word_count_kafka.sh example applications in the new module named wayang-applications.

This PR allows me to close this contribution with a partial result:
A working example of an application using Kafka Source and Kafka Sink in on JavaPlan in Wayang.

Moving forward, I will start with a new feature branch in a forked repository, to keep the main repository clean in the future.
The branch can be deleted after the merge.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

license-eye has checked 1554 files.

Valid Invalid Ignored Fixed
1420 1 133 0
Click to see the invalid file list
  • wayang-applications/bin/bootstrap.sh
Use this command to fix any missing license headers
```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

Comment thread wayang-applications/bin/bootstrap.sh Outdated
….12 in wayang-applications so that it can be build in cloud env. MIGHT BRING BACK THE VISIBILITY ISSUE ! -> must be checked
Comment thread wayang-applications/pom.xml Outdated
@juripetersen
Copy link
Copy Markdown
Contributor

@kamir do you think that creating a wayang-kafka module inside of wayang-platforms would be a cleaner approach to add these operators? Right now it seems like you edit both wayang-java and wayang-spark, where both of them contain versions of operators from Kafka. I think conceptually, having all implementations for the specific apis in wayang-kafka would be neat. Then you could have wayang-kafka/src/main/java/JavaKafkaOperator wayang-kafka/src/main/java/SparkKafkaOperator etc and the respective platforms themselves don't have to concern about Kafka.

What do you think?

@zkaoudi
Copy link
Copy Markdown
Contributor

zkaoudi commented Sep 5, 2024

That's an interesting idea @juripetersen. If we go that direction, we should do for all data sources, right? maybe a parent module wayang-datasources?

@kamir
Copy link
Copy Markdown
Contributor Author

kamir commented Sep 5, 2024

@kamir do you think that creating a wayang-kafka module inside of wayang-platforms would be a cleaner approach to add these operators? Right now it seems like you edit both wayang-java and wayang-spark, where both of them contain versions of operators from Kafka. I think conceptually, having all implementations for the specific apis in wayang-kafka would be neat. Then you could have wayang-kafka/src/main/java/JavaKafkaOperator wayang-kafka/src/main/java/SparkKafkaOperator etc and the respective platforms themselves don't have to concern about Kafka.

What do you think?

Yes, this is a great idea. I was thinking about the issue of "double implementation" but did not make so far towards a solution.
Yes, lets do this.

To get ready for this task I suggest, that I first finish the demo, and the blog.
We all together nail the release 1.0.
And after we reached the 1.0 level, we make this restructuring and release it as 1.1.

What do you think?

@juripetersen
Copy link
Copy Markdown
Contributor

I'd say if that is the pragmatic solution, let's go for it. But knowing that the restructure won't happen for a long time, let's try to create an issue addressing it so we will resolve that technical debt eventually.

@kamir kamir requested a review from juripetersen September 5, 2024 09:16
@kamir
Copy link
Copy Markdown
Contributor Author

kamir commented Sep 5, 2024

Issue is created:

#469

@zkaoudi
Copy link
Copy Markdown
Contributor

zkaoudi commented Sep 5, 2024

But just to make sure we are all on the same page: Is the code for a Kafka source operator and a Java source operator the same? For example, for the TextFileSource it's not the same code.

@kamir
Copy link
Copy Markdown
Contributor Author

kamir commented Sep 5, 2024

The SparkPlatform handles the Kafka Source differently, using its own Spark-Kafka-Source.
Our JavaPlatform works directly with Consumer, and with Producer API. Hence we will have different implementations for this particular Problem.

My understanding was, that all Kafka related functionality should be "grouped" in a specific module, and there, the individual implementations for Java / Spark coexist, but no other place in the project Wayang project has a Kafka dependency.

@juripetersen
Copy link
Copy Markdown
Contributor

My understanding was, that all Kafka related functionality should be "grouped" in a specific module, and there, the individual implementations for Java / Spark coexist, but no other place in the project Wayang project has a Kafka dependency.

That is essentially what I meant, yes. But we will push that change into the future as part of #469.

Comment thread README.md Outdated
Comment thread bin/wayang-submit Outdated
Comment thread guides/develop-with-Wayang.md Outdated
Comment thread guides/ml-in-Wayang.md Outdated
Comment thread guides/tutorial.md Outdated
Comment thread guides/tutorial.md Outdated
Comment thread wayang-docs/src/main/resources/index.md Outdated
Comment thread wayang-docs/src/main/resources/index.md Outdated
Comment thread wayang-docs/src/main/resources/index.md Outdated
Copy link
Copy Markdown
Contributor

@juripetersen juripetersen left a comment

Choose a reason for hiding this comment

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

Most of the comments I made should be resolvable pretty quick. Let me know if something is unclear!

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

license-eye has checked 1555 files.

Valid Invalid Ignored Fixed
1420 2 133 0
Click to see the invalid file list
  • env_template.sh
  • wayang-applications/bin/env_template.sh
Use this command to fix any missing license headers
```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

Comment thread env_template.sh Outdated
Comment thread env_template.sh Outdated
Comment thread wayang-applications/bin/env_template.sh Outdated
@kamir
Copy link
Copy Markdown
Contributor Author

kamir commented Sep 8, 2024

All issues solved.

Test Scripts still work on my system.

@kamir
Copy link
Copy Markdown
Contributor Author

kamir commented Sep 8, 2024

Build runs in cloud environment.

@juripetersen
Copy link
Copy Markdown
Contributor

@kamir there is still one debug print in the wayang-submit. If you resolve that, I will approve the review.

@kamir
Copy link
Copy Markdown
Contributor Author

kamir commented Sep 9, 2024

The debugging line has been removed.

Copy link
Copy Markdown
Contributor

@juripetersen juripetersen left a comment

Choose a reason for hiding this comment

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

Thank you, looks good to me!

Comment thread wayang-applications/bin/bootstrap.sh Outdated
Comment thread wayang-applications/bin/bootstrap.sh Outdated
Comment thread wayang-applications/bin/cleanup.sh Outdated
Comment thread wayang-applications/bin/run_wordcount_main.sh Outdated
Comment thread wayang-applications/bin/run_wordcount_main.sh Outdated
Comment thread wayang-applications/data/case-study/README.md Outdated
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

license-eye has checked 1555 files.

Valid Invalid Ignored Fixed
1422 1 132 0
Click to see the invalid file list
  • wayang-applications/bin/env.demo1.sh
Use this command to fix any missing license headers
```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

Comment thread env_template.sh Outdated
@kamir kamir requested a review from novatechflow September 10, 2024 19:08
Comment thread env_template.sh Outdated
novatechflow
novatechflow previously approved these changes Sep 11, 2024
Copy link
Copy Markdown
Member

@novatechflow novatechflow left a comment

Choose a reason for hiding this comment

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

@juripetersen please review

Comment thread env_template.sh Outdated
Comment thread env_template.sh Outdated
Copy link
Copy Markdown
Contributor

@juripetersen juripetersen left a comment

Choose a reason for hiding this comment

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

There are still two files with "os specific" configurations. Let's remove them.

pragmatic solution for different environments. We can open an Issue to add templates for Linux or Windows. marking it as "good first commit" for new contributions. Thoughts?
add OSX reference
Comment thread env_template.sh Outdated
Comment thread env_template.sh Outdated
@kamir
Copy link
Copy Markdown
Contributor Author

kamir commented Sep 12, 2024

The OSX specific paths have been removed. We will provide some specific scripts for WIN, OSX, Linux soon.

@juripetersen
Copy link
Copy Markdown
Contributor

@2pk03 @kamir I approved the review. I am not sure why this still requires one more review by someone.

Copy link
Copy Markdown
Member

@novatechflow novatechflow left a comment

Choose a reason for hiding this comment

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

Awesome, well done @kamir and @juripetersen!

@novatechflow novatechflow merged commit 886938e into main Sep 12, 2024
@novatechflow novatechflow deleted the kamir-patch-2 branch September 12, 2024 13:35
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.

4 participants