Skip to content
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

Add sample application and README file #10

Merged
merged 11 commits into from
Sep 30, 2021
Merged

Add sample application and README file #10

merged 11 commits into from
Sep 30, 2021

Conversation

JatinSanghvi
Copy link
Collaborator

@JatinSanghvi JatinSanghvi commented Sep 8, 2021

Checking the PR branch in forked repo: https://github.com/JatinSanghvi/keda-external-scaler-azure-cosmos-db (including the README file) might provide a better sense of the changes that were made

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)

Fixes

  • Addresses issue Provide GitHub Action to publish official image on GHCR #9 (need review to confirm)

  • Scales targets based on count of Cosmos DB container's partition-ranges with non-zero pending items. Existing implementation scales according to the estimate of items pending to be processed across all partitions, which is incorrect. Cosmos DB architecture limits the degree of parallelism of "processing new changes to the container" to the "number of logical partition ranges" (More information). The logical partition ranges is dependent on the physical partitions, which is turn depends on combination of container data storage size and container throughput (More information). For small containers with low throughput provisioned, we expect the scale targets to not scale beyond 1.

  • Fixes generation of metric names. Existing implementation generates the metric name in method GetMetricSpec but uses metric name from request metadata in method GetMetrics. If metric name is not specified in ScaledObject definition, returning metric name in GetMetricSpec won't make the metric-name part of scaler metadata when GetMetrics is called. On other hand, if the metric name is supplied by user in ScaledObject definition, GetMetricSpec would override the name, and KEDA won't be able to get the metrics for the generated metric-name when calling GetMetrics. New implementation provides consistent behavior across methods. All methods will reuse the metric-name if provided by user, else generate one based on other 'required' properties in ScaledObject.

  • Bases metric name generation based on lease container properties. The current metric-name generation logic will create same metric name if two different lease containers are created to monitor the same container.

  • Uses standard names as in Cosmos DB documentation for trigger properties. Connection instead ConnectionString (matches with other KEDA scalers), ContainerId instead of CollectionName and ProcessorName instead of LeaseCollectionPrefix.

Other changes

  • Adds sample producer and consumer projects with steps in README file to test scaling of target application between 0, 1 and 2 replicas.
  • Refactors code and updates unit tests.

Closes #2
Closes #3
Closes #9

Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
.gitignore Show resolved Hide resolved
Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
@JatinSanghvi
Copy link
Collaborator Author

Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
@JatinSanghvi
Copy link
Collaborator Author

@ahmelsayed, @tomkerkhove - Can you please review the PR?

@JatinSanghvi
Copy link
Collaborator Author

@ahmelsayed, @tomkerkhove - Gentle reminder for PR review.

@tomkerkhove
Copy link
Member

I just got back from holiday and will review asap

@tomkerkhove tomkerkhove self-assigned this Sep 20, 2021
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

I did a first skip but have a few high-level remarks:

  • The README does not provide documentation on what the scaler itself does and how it helps the customer. You should provide guidance on what it is, how to deploy and how to configure a ScaledObject
  • The scaler project was renamed, what was the reasoning for this as the previous naming convention (CosmosDb.Scaler) was a lot better imo
  • The repo needs a bit more structure, can you please introduce the following folders please?
    • src/ for the external scaler code
    • samples/ for the sample code and README to provide guidance how to run the sample
  • We should explicitly document that this is purely for Changefeed and not the rest

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
@JatinSanghvi
Copy link
Collaborator Author

@tomkerkhove - Please review the updated PR when you have time.

  • Added main README file with scalar overview, deployment instructions and trigger metadata details. Moved earlier README for sample app to sub-directory.
  • Introduced separate GitHub actions for different triggers to avoid confusion and ensure the built package versions do not overlap.
  • Updated namespace names from Keda.CosmoDbScaler to Keda.CosmoDb.Scaler.
  • Updated folder structure. The sample app is moved to /src/Scaler.Demo as that made more sense to me and helped retaining all project references in the solution file.

Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM, a big improvement - Thanks a ton! End-users can now better understand how it works and if it's a fit for them.

Just made a remark on streamlining the SO metadata but rest looks fine 👌

src/Scaler.Demo/Shared/CosmosDbConfig.cs Show resolved Hide resolved
src/Scaler.Demo/Shared/CosmosDbConfig.cs Show resolved Hide resolved
src/Scaler/Services/ScalerMetadata.cs Show resolved Hide resolved
deploy/deploy-scaledobject.yaml Show resolved Hide resolved
@tomkerkhove
Copy link
Member

@pragnagopa @ealsur Please take a final look

@tomkerkhove tomkerkhove removed their assignment Sep 28, 2021
Copy link

@pragnagopa pragnagopa left a comment

Choose a reason for hiding this comment

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

LGTM !

Signed-off-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com>
@tomkerkhove tomkerkhove merged commit 7928dc0 into kedacore:main Sep 30, 2021
@tomkerkhove
Copy link
Member

Ha, I have missed the last commit @JatinSanghvi.

Would it be possible to change ghcr.io/kedacore/keda-cosmosdb-scaler back to ghcr.io/kedacore/keda-external-scaler-azure-cosmos-db please?

This is important for our scaler approach where people need to understand that it's an external scaler and follow the convention. THank you!

@pragnagopa
Copy link

This is important for our scaler approach where people need to understand that it's an external scaler and follow the convention. THank you!

@JatinSanghvi - please use same name in mcr as well.

@JatinSanghvi
Copy link
Collaborator Author

JatinSanghvi commented Sep 30, 2021

@JatinSanghvi - please use same name in mcr as well.

Sure @pragnagopa, I was going to use the same name to avoid chance of confusion.

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.

Provide GitHub Action to publish official image on GHCR Provide README Provide CI
4 participants