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

[azdatalake][STG84] Append with flush #22245

Merged
merged 34 commits into from
Jan 17, 2024
Merged

[azdatalake][STG84] Append with flush #22245

merged 34 commits into from
Jan 17, 2024

Conversation

tanyasethi-msft
Copy link
Member

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to module CHANGELOG.md are included.
  • MIT license headers are included in each file.

Sorry, something went wrong.

RickWinter and others added 30 commits January 4, 2024 09:46
* Update package versions

* upgrade to MSAL v1.2.1

* add token_type to fake tokens

* revert debugging

---------

Co-authored-by: Charles Lowell <[email protected]>
* link current instead of depricated path

* remove newline at end of file
* More azappconfig cleanup

Update BeginCreateSnapshot to return the complete Snapshot type.
Remove unused response types.
Added some missed error checking in a few tests.

* fix a few doc comments
…ons (#22153)

The processor load balancer had a bug where a processor would not claim partitions even if it didn't yet have the proper share.

Added in stress tests (balance, multibalance) to run several scenarios with our supported strategies and different parallel owners.

Fixes #22097
Use azcore.ETag instead of string for public surface area.
Add doc comments for non-standard ETag option names.
…22210)

Fixing issue where you can't use whisper with m4a files.

* It's one of the formats that doesn't seem to be recognized without an explicit file extension, which you can pass via Filename
* My tests were too heavily dependent on implementation details of the models. Changing this out to check that things are working correctly without checking the exact contents of the response.
* Also, rerecorded tests since we're doing multiple audio tests as well.

Fixes #22195
Missed one in last round of clean-up.
Will tidy all modules under the current working directory or under the
specified directory.
#22154)

Updating batching to allow for a configurable wait time. Can lead to fuller batches for people that want to tune it.

Fixes #19172
* check for the presence of a compatible powershell. ensure that we always return a list of tags

* allow the script to require pshell6+

* remove the en-us from the link

---------

Co-authored-by: Scott Beddall (from Dev Box) <[email protected]>
* upgrade to latest codegen

* merge resolver

* enable span and add changelog

* update module name
* fix generator tool

* gofmt

* fix

* goimports
@@ -243,6 +243,8 @@ type AppendDataOptions struct {
LeaseAccessConditions *LeaseAccessConditions
// CPKInfo contains optional parameters to perform encryption using customer-provided key.
CPKInfo *CPKInfo
//Flush Optional. If true, the file will be flushed after append.
Flush *bool
Copy link
Member

Choose a reason for hiding this comment

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

If Flush: nil is semantically equivalent to Flush: false then it would be better to define this as Flush: bool as that's easier to work with.

Copy link
Member Author

Choose a reason for hiding this comment

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

We usually use pointers for optional boolean parameters, like here. Also, this Flush parameter is defined as a pointer in generated code.
Happy to discuss if a change is needed in this approach.

Copy link
Member

Choose a reason for hiding this comment

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

Agree "true" looks better and more user friendly then "to.Ptr(true)". Are there other places where we send true/false in form of pointers ?

Copy link
Member

Choose a reason for hiding this comment

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

I see that auto generated code itself take *bool and hence our wrappers have also taken dependency on similar types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes @vibhansa-msft, everywhere we send true/false in form of pointers, eg - Link

Copy link
Member

Choose a reason for hiding this comment

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

There is a check where if flush is nil, it doesn't add the query parameter in the request URL (source).
If we have this as pointer, it will prevent adding this query parameter. Otherwise, it will add flush=false every time if user has not set this parameter.

Copy link
Member

Choose a reason for hiding this comment

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

If omitting it is important, then go ahead and use *bool.

@tanyasethi-msft tanyasethi-msft changed the base branch from main to feature/azdatalake/STG83-90 January 17, 2024 03:35
@tanyasethi-msft tanyasethi-msft merged commit a8c6499 into Azure:feature/azdatalake/STG83-90 Jan 17, 2024
35 checks passed
tanyasethi-msft added a commit that referenced this pull request Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet