Review result reaction #111

ruokun-niu · 2024-11-07T22:55:21Z

Description

The result reaction is used to retrieve the current result set, or the result set at a given timestamp.

To retrieve the current result set, send a GET request to the endpoint /{query_id}, where query_id is the query you are interested in.

For example:

curl -X GET "localhost:8080/hello-world-from"

To retrieve the result set at a given timestamp, send a GET request to the endpoint /{query_id}/{timestamp}, where query_id is the query you are interested in, and timestamp is the timestamp you are interested in. (This requires the retentionPolicy of the continuous query to be set to all or expire
For example:

curl -X GET "localhost:8080/hello-world-from/123456789"

Sample output can be found here: https://gist.github.com/ruokun-niu/f5be829bdc6c82e332e99725c923a5e2

Type of change

This pull request fixes a bug in Drasi and has an approved issue (issue link required).

Fixes: #issue_number

reactions/platform/result-reaction/Services/ResultViewClient.cs

dev-tools/testing/quick-test-environment/quick-test-environment.ps1

@@ -37,7 +37,7 @@
 drasi wait reaction quick-result-reaction -t 120

 # Run the kubectl command and capture the output
-$initial_output = & kubectl run curl-pod --image=curlimages/curl -n $namespace --restart=Never --rm --attach -q -- sh -c 'curl -s http://quick-result-reaction-gateway:8080/quick-query/all' 2>$null
+$initial_output = & kubectl run curl-pod --image=curlimages/curl -n $namespace --restart=Never --rm --attach -q -- sh -c 'curl -X GET -s http://quick-result-reaction-gateway:8080/quick-query' 2>$null


dev-tools/testing/quick-test-environment/quick-test-environment.ps1

@@ -72,7 +72,7 @@

 Start-Sleep -Seconds 20

-$final_output = & kubectl run curl-pod --image=curlimages/curl -n $namespace --restart=Never --rm --attach -q -- sh -c 'curl -s http://quick-result-reaction-gateway:8080/quick-query/all' 2>$null
+$final_output = & kubectl run curl-pod --image=curlimages/curl -n $namespace --restart=Never --rm --attach -q -- sh -c 'curl -X GET -s http://quick-result-reaction-gateway:8080/quick-query' 2>$null


dev-tools/testing/quick-test-environment/quick-test-environment.sh

@@ -37,7 +37,7 @@
 drasi wait reaction quick-result-reaction -t 120

 # Initial result
-initial_output=$(kubectl run curl-pod --image=curlimages/curl -n $namespace --restart=Never  --rm --attach -q -- sh -c 'curl -s http://quick-result-reaction-gateway:8080/quick-query/all' 2>/dev/null)
+initial_output=$(kubectl run curl-pod --image=curlimages/curl -n $namespace --restart=Never  --rm --attach -q -- sh -c 'curl -X GET -s http://quick-result-reaction-gateway:8080/quick-query' 2>/dev/null)


dev-tools/testing/quick-test-environment/quick-test-environment.sh

@@ -56,7 +56,7 @@
 echo "Retrieving the current result from the debug reaction"
 sleep 20

-final_output=$(kubectl run curl-pod --image=curlimages/curl -n $namespace --restart=Never  --rm --attach -q -- sh -c 'curl -s http://quick-result-reaction-gateway:8080/quick-query/all' 2>/dev/null)
+final_output=$(kubectl run curl-pod --image=curlimages/curl -n $namespace --restart=Never  --rm --attach -q -- sh -c 'curl -X GET -s http://quick-result-reaction-gateway:8080/quick-query' 2>/dev/null)


danielgerlag · 2024-11-12T16:49:45Z

reactions/platform/result-reaction/Program.cs


 // Adding an endpoint that supports retrieving all results
-app.MapGet("/{queryId}/all", async (string queryId) => 
+app.MapGet("/{queryId}", async (string queryId) => 


You could make this support large results really easily by adding support for streaming results, but return an IAsyncEnumerable - https://anthonygiretti.com/2021/09/22/asp-net-core-6-streaming-json-responses-with-iasyncenumerable-example-with-angular/

resultViewClient.GetCurrentResult already returns a IAsyncEnumerable, so you could just simply return the result of that method.

Hi @danielgerlag , I have updated the endpoints to return an IAsyncEnumerable, but I realized that if we just return this it will also return the metadata. I think that sometimes users would just want to get the data (for example in the quick test), so I have created two additional routes: /{queryId}/data and /{queryId}/{ts}/data that just returns the data as IAsyncEnumerable<JsonElement>.

# Timestamp with only the data ruokunniu ~/Documents/playground $ curl -X GET "localhost:8080/hello-world-from/12323423423232/data" [{"MessageFrom":"Brian Kernighan","MessageId":2},{"MessageFrom":"Allen","MessageId":5},{"MessageFrom":"Allen","MessageId":6},{"MessageFrom":"Allen","MessageId":7}]% # Timestamp ruokunniu ~/Documents/playground $ curl -X GET "localhost:8080/hello-world-from/12323423423232" [{"header":{"sequence":10,"timestamp":1731703800526,"state":"running"}},{"data":{"MessageFrom":"Brian Kernighan","MessageId":2}},{"data":{"MessageFrom":"Allen","MessageId":5}},{"data":{"MessageFrom":"Allen","MessageId":6}},{"data":{"MessageFrom":"Allen","MessageId":7}}]% # regular query ruokunniu ~/Documents/playground $ curl -X GET "localhost:8080/hello-world-from" [{"header":{"sequence":10,"timestamp":1731703800526,"state":"running"}},{"data":{"MessageFrom":"Brian Kernighan","MessageId":2}},{"data":{"MessageFrom":"Allen","MessageId":5}},{"data":{"MessageFrom":"Allen","MessageId":6}},{"data":{"MessageFrom":"Allen","MessageId":7}}]% # query with data ruokunniu ~/Documents/playground $ curl -X GET "localhost:8080/hello-world-from/data" [{"MessageFrom":"Brian Kernighan","MessageId":2},{"MessageFrom":"Allen","MessageId":5},{"MessageFrom":"Allen","MessageId":6},{"MessageFrom":"Allen","MessageId":7}]%

I'll update the quick test based on this

reactions/platform/result-reaction/Program.cs

reactions/platform/result-reaction/Services/ResultViewClient.cs

+			Stream? stream = null;
+			try
+			{
+				stream = await _httpClient.GetStreamAsync($"http://{queryContainerId}-view-svc/{queryId}", cancellationToken);


reactions/platform/result-reaction/Services/ResultViewClient.cs

+			Stream? stream = null;
+			try
+			{
+				stream = await _httpClient.GetStreamAsync($"http://{queryContainerId}-view-svc/{queryId}?timestamp={ts}", cancellationToken);


danielgerlag · 2024-11-20T04:10:15Z

reactions/platform/result-reaction/result-reaction.csproj

-    <PackageReference Include="Dapr.AspNetCore" Version="1.10.0" />
-    <PackageReference Include="Dapr.Client" Version="1.10.0" />
+    <PackageReference Include="Dapr.AspNetCore" Version="1.14.0" />
+    <PackageReference Include="Dapr.Client" Version="1.14.0" />


I think we can remove all the Dapr libraries.

danielgerlag · 2024-11-20T04:11:20Z

reactions/platform/result-reaction/Program.cs

+var builder = WebApplication.CreateBuilder(args);
+var configuration = BuildConfiguration();
+
+var queryContainerId = configuration["QueryContainerId"] ?? "default";


Is this one of the reaction config properties?

Since the result reaction does not use the NuGet Reaction SDK, I did not modify how it retrieves the query container ID.

I think this was originally a hack. If we keep it, we need to document this as a config property and that only queries within that container would work.

reactions/platform/result-reaction/Program.cs

danielgerlag · 2024-12-19T22:55:11Z

reactions/platform/result-reaction/Program.cs

+var app = builder.Build();
+app.UseRouting();
+
+app.Urls.Add("http://0.0.0.0:80");  //dapr


We should have the dapr endpoint and query endpoint hosted by 2 different web servers within the app. The query endpoint will be exposed to the public, and if you call it with the right routes, you can pretend to be the dapr side car.

reactions/platform/result-reaction/Program.cs

danielgerlag · 2024-12-20T04:43:09Z

reactions/platform/result-reaction/Program.cs

 app.Urls.Add("http://0.0.0.0:8080"); //app

+// Dapr server
+var daprBuilder = WebApplication.CreateBuilder(args);
+var daprApp = daprBuilder.Build();


Which app side dapr APIs are we implementing for this reaction?

Hmm I don't think we are implementing anything for this reaction? Was there something specific that stood out to you?

If we aren't implementing any app side dapr APIs, then we don't need to have an open port for it at all?

Right good point. I just removed this and pointed dapr to the app port 8080

i think we can just remove the dapr app port?

Yep that's what I did. I meant that I had to configure the port in the reaction provider, otherwise the sidecar container will fail to start

I think if no app port is specified, then the sidecar will not try to connect to the app, and the sidecar should always start?

I think the issue with not setting an app port is that by default port 80 is used as the app port in the k8s provider (https://github.com/drasi-project/drasi-platform/blob/main/control-planes/kubernetes_provider/src/spec_builder/reaction.rs#L155).

One possible approach is to update this section of code and use None as the default value (indicating that we do not want sidecar to be enabled for this particular reaction). Although this might require changes in other reactions/reactionproviders. Perhaps this could be a separate task @danielgerlag ?

Good point, I think that's out of scope for this PR... let's circle back later.

reactions/platform/result-reaction/Program.cs

+var app = builder.Build();
+app.UseRouting();
+
+app.Urls.Add("http://0.0.0.0:8080"); //app


ruokun-niu added 2 commits November 7, 2024 13:27

result reaction

899f83a

completed Result reaction

0bbc672

github-advanced-security bot found potential problems Nov 7, 2024

View reviewed changes

reactions/platform/result-reaction/Services/ResultViewClient.cs Fixed Show fixed Hide fixed

updated quick test

6db95c1

github-advanced-security bot found potential problems Nov 8, 2024

View reviewed changes

ruokun-niu marked this pull request as ready for review November 8, 2024 00:19

ruokun-niu requested a review from a team as a code owner November 8, 2024 00:19

ruokun-niu requested review from amansinghoriginal, danielgerlag and agentofreality November 8, 2024 00:20

ruokun-niu added 3 commits November 7, 2024 16:21

updated dependency

961fe61

nit

9698eff

Merge branch 'main' into review-result-reaction

2263683

danielgerlag reviewed Nov 12, 2024

View reviewed changes

updated route to return Iasyncenumerable

feacd93

github-advanced-security bot found potential problems Nov 15, 2024

View reviewed changes

update quicktest to reflect the new endpoints

29dfb83

danielgerlag reviewed Nov 20, 2024

View reviewed changes

ruokun-niu added 2 commits November 20, 2024 10:40

Merge branch 'main' into review-result-reaction

fce9465

remove dapr dependency

ad4bb18

danielgerlag reviewed Nov 21, 2024

View reviewed changes

reactions/platform/result-reaction/Program.cs Show resolved Hide resolved

ruokun-niu added 2 commits November 21, 2024 10:52

added querycontainerID as a config

87c15ef

Merge branch 'main' into review-result-reaction

b29885d

ruokun-niu requested a review from danielgerlag December 19, 2024 18:49

danielgerlag reviewed Dec 19, 2024

View reviewed changes

configured dapr endpoint and added queryid check

aa69107

github-advanced-security bot found potential problems Dec 20, 2024

View reviewed changes

reactions/platform/result-reaction/Program.cs Fixed Show fixed Hide fixed

reactions/platform/result-reaction/Program.cs Fixed Show fixed Hide fixed

nit

d11b454

danielgerlag reviewed Dec 20, 2024

View reviewed changes

removed open port for dapr

ebe8e1f

github-advanced-security bot found potential problems Jan 2, 2025

View reviewed changes

reactions/platform/result-reaction/Program.cs

var app = builder.Build();

app.UseRouting();

app.Urls.Add("http://0.0.0.0:8080"); //app

Check warning

Code scanning / devskim

An HTTP-based URL without TLS was detected. Warning

Insecure URL

ruokun-niu added 2 commits January 3, 2025 09:26

Merge branch 'main' into review-result-reaction

e6a3053

Merge branch 'main' into review-result-reaction

d81c696

danielgerlag approved these changes Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review result reaction #111

Review result reaction #111

ruokun-niu commented Nov 7, 2024 •

edited

Loading

danielgerlag Nov 12, 2024

danielgerlag Nov 12, 2024

ruokun-niu Nov 15, 2024 •

edited

Loading

ruokun-niu Nov 15, 2024

ruokun-niu Nov 15, 2024

danielgerlag Nov 20, 2024

danielgerlag Nov 20, 2024

ruokun-niu Nov 20, 2024

danielgerlag Nov 21, 2024

danielgerlag Dec 19, 2024

danielgerlag Dec 20, 2024

ruokun-niu Dec 20, 2024

danielgerlag Jan 2, 2025

ruokun-niu Jan 2, 2025

danielgerlag Jan 2, 2025

ruokun-niu Jan 2, 2025

danielgerlag Jan 2, 2025

ruokun-niu Jan 3, 2025

danielgerlag Jan 3, 2025

Review result reaction #111

Are you sure you want to change the base?

Review result reaction #111

Conversation

ruokun-niu commented Nov 7, 2024 • edited Loading

Description

Type of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruokun-niu Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruokun-niu commented Nov 7, 2024 •

edited

Loading

ruokun-niu Nov 15, 2024 •

edited

Loading