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

Streaming ingest #63

Merged
merged 39 commits into from
Feb 14, 2022
Merged

Streaming ingest #63

merged 39 commits into from
Feb 14, 2022

Conversation

AsafMah
Copy link
Contributor

@AsafMah AsafMah commented Dec 22, 2021

-Upgrade go version to 1.16 (1.13 is OOL for a while, and we need new features)
-Adding streaming ingest client with the same ability as ingest.
-Deprecating the Stream method inside ingest
-"Modernize" e2e testing -

  • Replaced "github.com/kylelemons/godebug/pretty" with testify because:
    • pretty is subtly wrong - it will show an object and a pointer to the object as equal
    • The output of testify is easier to read, for both humans and for IDEs (goland for example can understand the diff)
    • pretty is pretty much dead
  • Making the tests run with Subtests (now you can know which test in the table failed easily)
  • Later we can go over the rest of the code and modernize other tests

Still needed:

  • Docs and samples

@AsafMah AsafMah marked this pull request as draft December 22, 2021 08:47
@github-actions
Copy link

github-actions bot commented Dec 29, 2021

Unit Test Results

    1 files  ±  0    13 suites  ±0   11m 26s ⏱️ + 7m 12s
108 tests +35  108 ✔️ +35  0 💤 ±0  0 ±0 

Results for commit b1eeb26. ± Comparison against base commit b89f168.

♻️ This comment has been updated with latest results.

@AsafMah AsafMah marked this pull request as ready for review January 4, 2022 09:26
@AsafMah AsafMah requested a review from yogilad January 4, 2022 09:26
kusto/ingest/file_options.go Show resolved Hide resolved

func (o option) Run(p *properties.All, scopes OptionScope) error {

for scope, scopeName := range scopesMap {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you running on scopeMap?
Should it not be a simple x & y check?
I think the issue could be you put the source scope and protocol scope at the same enum

Copy link
Contributor

Choose a reason for hiding this comment

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

There's an assumption here that scopes include one Source scope and one Protocol scope.
I'm guessing the loop is for producing a specific error, but this can be resolved if you would pass them as separate elements and check the matching individually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done
Let me know if that what you had in mind

kusto/ingest/file_options.go Outdated Show resolved Hide resolved
kusto/ingest/file_options.go Show resolved Hide resolved
kusto/ingest/file_options.go Outdated Show resolved Hide resolved
kusto/ingest/ingest.go Outdated Show resolved Hide resolved
kusto/ingest/internal/conn/conn.go Show resolved Hide resolved
kusto/ingest/status.go Outdated Show resolved Hide resolved
kusto/ingest/streaming_ingest.go Outdated Show resolved Hide resolved
kusto/ingest/streaming_ingest.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2022

Codecov Report

Merging #63 (b1eeb26) into master (02c5c0e) will increase coverage by 2.12%.
The diff coverage is 37.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   50.29%   52.42%   +2.12%     
==========================================
  Files          28       30       +2     
  Lines        4096     4273     +177     
==========================================
+ Hits         2060     2240     +180     
+ Misses       1890     1871      -19     
- Partials      146      162      +16     
Impacted Files Coverage Δ
kusto/ingest/internal/filesystem/filesystem.go 26.80% <0.00%> (-1.12%) ⬇️
kusto/ingest/result.go 6.95% <0.00%> (+6.95%) ⬆️
kusto/ingest/status.go 17.16% <0.00%> (+17.16%) ⬆️
kusto/mock.go 50.00% <0.00%> (-13.77%) ⬇️
kusto/ingest/file_options.go 25.89% <25.89%> (ø)
kusto/ingest/ingest.go 44.44% <56.00%> (+39.11%) ⬆️
kusto/ingest/streaming.go 63.21% <63.21%> (ø)
kusto/ingest/internal/conn/conn.go 66.10% <81.25%> (+0.36%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b89f168...b1eeb26. Read the comment docs.

# Conflicts:
#	kusto/ingest/ingest.go
#	kusto/test/etoe/etoe_test.go
kusto/ingest/file_options.go Outdated Show resolved Hide resolved
kusto/ingest/file_options.go Outdated Show resolved Hide resolved
@AsafMah AsafMah merged commit be42011 into master Feb 14, 2022
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.

3 participants