Skip to content

Support detect spring-integration-eventhubs, spring-messaging-eventhubs, and spring-kafka#85

Merged
rujche merged 19 commits into
azure-javaee:feature/sjadfrom
haoozhang:haozhan/eh-integration
Dec 31, 2024
Merged

Support detect spring-integration-eventhubs, spring-messaging-eventhubs, and spring-kafka#85
rujche merged 19 commits into
azure-javaee:feature/sjadfrom
haoozhang:haozhan/eh-integration

Conversation

@haoozhang

@haoozhang haoozhang commented Dec 18, 2024

Copy link
Copy Markdown

Support to detect:

  1. spring-integration-eventhubs
  2. spring-messaging-eventhubs
  3. spring-kafka

Still have issues:

  1. for binder-eventhubs:
    • consume group must be $Default, because we didn't detect this currently.
  2. multi-binders-evnethubs:
    • storage account configurations have different prefixes, so cannot work in this case.
  3. spring-kafka:
    • need spring.kafka.bootstrap-servers, it will conflict with binder-kafka
    • support user managed identity only, how to configure connectionString

@haoozhang
haoozhang marked this pull request as ready for review December 19, 2024 08:09
Comment thread cli/azd/internal/appdetect/appdetect.go Outdated
Comment thread cli/azd/internal/appdetect/appdetect.go Outdated
Comment thread cli/azd/internal/appdetect/appdetect.go Outdated
Comment thread cli/azd/internal/appdetect/spring_boot.go Outdated
Comment thread cli/azd/internal/appdetect/spring_boot.go Outdated
Comment thread cli/azd/internal/repository/app_init.go Outdated
Comment thread cli/azd/internal/repository/app_init.go Outdated
Comment thread cli/azd/internal/repository/app_init.go Outdated
Comment thread cli/azd/internal/repository/app_init.go Outdated
@haoozhang
haoozhang requested a review from rujche December 25, 2024 06:24
Comment thread cli/azd/internal/appdetect/spring_boot.go
Comment thread cli/azd/internal/appdetect/spring_boot.go
Comment thread cli/azd/internal/appdetect/spring_boot.go
Comment thread cli/azd/internal/repository/app_init.go Outdated
func promptMissingEventHubsNameOrExit(console input.Console, ctx context.Context, eventHubs *appdetect.AzureDepEventHubs) {
switch eventHubs.MavenDependencyType {
case appdetect.SpringIntegrationEventHubs, appdetect.SpringMessagingEventHubs, appdetect.SpringKafka:
eventHubsNames, err := promptEventHubsNames(console, ctx)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In my understanding, in this case, the event hub name can be detected in customer's project's java code, but we haven't implement this feature now. Because the eventhub name will be used in customer's project. Is this right?

If it can be detected by analyzing java code, it means promptEventHubsNames can be removed after we provide this feature. Is this right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, you are exactly right.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actually, we already have some code analyzed java codes:

image

When implement the feature about detecting event hub name in java code, please consider whether these java analyzing codes can be moved to a central place like spring_boot.go.

Comment thread cli/azd/internal/appdetect/spring_boot.go Outdated
Comment thread cli/azd/internal/repository/app_init.go
haozhang added 3 commits December 30, 2024 14:40
…tegration

# Conflicts:
#	cli/azd/internal/appdetect/spring_boot.go
Comment thread cli/azd/internal/repository/app_init.go Outdated
Comment thread cli/azd/internal/appdetect/appdetect.go Outdated
Comment thread cli/azd/internal/appdetect/appdetect_test.go
Comment thread cli/azd/internal/appdetect/appdetect_test.go
azureDep.EventHubsNamePropertyMap[property] = eventHubsName
}
azureDep.FromDependency = append(azureDep.FromDependency, newDep.FromDependency...)
azureDep.UseKafka = newDep.UseKafka

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it possible that current func executed twice and newDep.UseKafka=true in the first time and newDep.UseKafka=false in the second time?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good point. After deprecating the UseKafka property, it won't be impacted.

@rujche rujche Dec 31, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What's the expected behavior for above case? Will useKafka func return true or false? Is it possible that customer's application use EventHub for Kafka and EventHub at the same time in one app?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The expected behavior is that this property should be true as long as kafka used.
So if it is executed twice and newDep.UseKafka=true in the first time and newDep.UseKafka=false in the second time, it will return wrong result.
However, after we deprecate this property and use a func to replace it (as mentioned in the comment #85 (comment)), we can avoid this problem, just check this by checking the DependencyTypes.

Comment thread cli/azd/internal/appdetect/spring_boot.go Outdated

@rujche rujche left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'll approve & merge current PR. Please help to confirm this: Is it possible that customer's application use EventHub for Kafka and EventHub at the same time in one app?

@rujche
rujche merged commit 60f57d0 into azure-javaee:feature/sjad Dec 31, 2024
@haoozhang

Copy link
Copy Markdown
Author

I'll approve & merge current PR. Please help to confirm this: Is it possible that customer's application use EventHub for Kafka and EventHub at the same time in one app?

I verified that user can use Event Hubs and Event Hubs for Kafka at the same time.
See this sample code: https://github.com/haoozhang/eventhubs-and-eventhubs-for-kafka

@rujche

rujche commented Jan 3, 2025

Copy link
Copy Markdown

I'll approve & merge current PR. Please help to confirm this: Is it possible that customer's application use EventHub for Kafka and EventHub at the same time in one app?

I verified that user can use Event Hubs and Event Hubs for Kafka at the same time. See this sample code: https://github.com/haoozhang/eventhubs-and-eventhubs-for-kafka

@haoozhang

Thanks for the feedback.

  1. Can SJAD deploy your sample application and make your sample application work well?
  2. Could you please move your sample application into this repo: https://github.com/azure-javaee/azure-spring-boot-samples

@haoozhang

Copy link
Copy Markdown
Author

I'll approve & merge current PR. Please help to confirm this: Is it possible that customer's application use EventHub for Kafka and EventHub at the same time in one app?

I verified that user can use Event Hubs and Event Hubs for Kafka at the same time. See this sample code: https://github.com/haoozhang/eventhubs-and-eventhubs-for-kafka

@haoozhang

Thanks for the feedback.

  1. Can SJAD deploy your sample application and make your sample application work well?
  2. Could you please move your sample application into this repo: https://github.com/azure-javaee/azure-spring-boot-samples

@rujche
SJAD currently can NOT make the application work well, because it identifies the application uses the Kafka, so the azure.yaml will contain messaging.kafka (but not contain messaging.eventhubs). Then when azd up later, it will get the Envs based on messaging type, it will only get the Envs for Kafka, but cannot get the necessary Envs for Eventhubs.

@rujche

rujche commented Jan 6, 2025

Copy link
Copy Markdown

@haoozhang
So it's a new feature to support messaging.kafka and messaging.eventhubs at the same time in SJAD. Could you please create an ADO item for this feature?

I guess after support this new feature in SJAD, isKafka is not necessary anymore because kafka and eventhub will be viewed as different thing.

@haoozhang

Copy link
Copy Markdown
Author

@haoozhang So it's a new feature to support messaging.kafka and messaging.eventhubs at the same time in SJAD. Could you please create an ADO item for this feature?

I guess after support this new feature in SJAD, isKafka is not necessary anymore because kafka and eventhub will be viewed as different thing.

@rujche Sure, let's track this enhancement with this item: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2332666/

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.

2 participants