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.

Sign up for GitHub

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

Jump to bottom

docs(core): add the description of version parameter for operator #5144

Open

meteorgan wants to merge 1 commit into apache:main from meteorgan:versioning-docs

Contributor

meteorgan commented Sep 25, 2024

Which issue does this PR close?

Part of #2611.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?


          docs(core): add the description of version parameter for operator

f6d222a

github-actions bot added the releases-note/docs label

meteorgan marked this pull request as ready for review

September 25, 2024 15:42

meteorgan requested a review from Xuanwo as a code owner

September 25, 2024 15:42

Contributor Author

meteorgan commented Sep 25, 2024

why is there no # before use opendal::Operator; at line 127 ? is a mistake or intentional ? do we have any guidelines for this ?

opendal/core/src/types/operator/operator.rs

Lines 122 to 133 in d479ac3

    
           /// # Examples 
        
           /// 
        
           /// ``` 
        
           /// # use std::sync::Arc; 
        
           /// # use anyhow::Result; 
        
           /// use opendal::Operator; 
        
           /// 
        
           /// # async fn test(op: Operator) -> Result<()> { 
        
           /// let info = op.info(); 
        
           /// # Ok(()) 
        
           /// # } 
        
           /// ```

Member

Xuanwo commented Sep 27, 2024

why is there no # before use opendal::Operator; at line 127 ? is a mistake or intentional ? do we have any guidelines for this ?

opendal/core/src/types/operator/operator.rs

Lines 122 to 133 in d479ac3

/// # Examples

///

/// ```

/// # use std::sync::Arc;

/// # use anyhow::Result;

/// use opendal::Operator;

///

/// # async fn test(op: Operator) -> Result<()> {

/// let info = op.info();

/// # Ok(())

/// # }

/// ```

I believe it's intentional. We don't have guidelines for this. Please feel free to write in your preferred way.

Xuanwo reviewed

View reviewed changes

core/src/types/operator/operator.rs

+                  ///
+                  /// Set `version` for this `stat` request.
+                  ///
+                  /// This feature can be used to retrieve the file metadata that matches a specified version.

Member

Xuanwo Sep 27, 2024

Hi, the way about discussing version is a bit confusing to me. Unlike ETag, Version is not used for matching. Instead, we request the specified version directly. We will return NotFound if the requested version does not exist for the given path.

Contributor Author

meteorgan Oct 2, 2024 •

edited

Loading

how about

    /// This feature can be used to retrieve the path's metadata of a specific version.
    ///
    /// If the version doesn't exist, an error with kind [`ErrorKind::NotFound`] will be returned.

core/src/types/operator/operator.rs

+                  ///
+                  /// This feature can be used to retrieve the file metadata that matches a specified version.
+                  ///
+                  /// If file exists, but the version doesn't match, an error with kind [`ErrorKind::NotFound`]

Member

Xuanwo Sep 27, 2024

the same.

core/src/types/operator/operator.rs

+                  /// By default, OpenDAL reads a file using the current version. By setting the `version`, OpenDAL will read
+                  /// the file with the specified version.
+                  ///
+                  /// If the file exists, but the version doesn't match, an error with kind [`ErrorKind::NotFound`]

Member

Xuanwo Sep 27, 2024

the same.

core/src/types/operator/operator.rs

+                  /// By default, OpenDAL reads a file using the current version. By setting the `version`, OpenDAL will read
+                  /// the file with the specified version.
+                  ///
+                  /// If the file exists, but the version doesn't match, an error with kind [`ErrorKind::NotFound`]

Member

Xuanwo Sep 27, 2024

the same.

core/src/types/operator/operator.rs

Comment on lines +1347 to +1351

+                  /// If `versioning` is not enabled, a `delete` request will permanently remove the file.
+                  ///
+                  /// when `versioning` is enabled, a simple `delete` request won't permanently remove the file,
+                  /// it can still be accessed using its version.
+                  /// By setting the `version`, OpenDAL will permanently remove the file with the specified version.

Member

Xuanwo Sep 27, 2024

Hi, it's better not to discuss "permanently delete" here. This behavior is determined by the service itself.

Contributor Author

meteorgan Oct 2, 2024

how about

    /// Set `version` for this `delete` request.
    ///
    /// remove a specific version of the given path.
    ///
    /// If the version doesn't exist, OpenDAL will not return errors.

core/src/types/operator/operator.rs

@@ @@ -1636,6 +1719,24 @@ impl Operator { @@
                   /// # }
                   /// ```
                   ///
+                  /// ## `version`
+                  ///
+                  /// Specify whether to list all object versions or not

Member

Xuanwo Sep 27, 2024

The behavior we implemented is "list files along with their versions." Please always remember that OpenDAL's vision is "accessing data freely." We should do our best to distinguish OpenDAL's API from the underlying storage services' implementation details.

Contributor Author

meteorgan Oct 2, 2024

how about

    /// ## `version`
    ///
    /// Specify whether to list files along with all their versions or not
    ///
    /// if `version` is not enabled, or is set to false, only the current files will be returned.
    ///
    /// if `version` is set to true, all the file versions will be returned.

core/src/types/operator/operator.rs

+                  ///
+                  /// Specify whether to list all object versions or not
+                  ///
+                  /// if `version` is not set, or is set to false, only the active files with the current version will be returned.

Member

Xuanwo Sep 27, 2024

if version is not set, or is set to false,

If version is not enabled?

core/src/types/operator/operator.rs

+                  ///
+                  /// if `version` is not set, or is set to false, only the active files with the current version will be returned.
+                  ///
+                  /// when `version` is set to true and the backend service has `versioning` enabled, all object versions will be returned.

Member

Xuanwo Sep 27, 2024

As previously discussed, please refrain from disclosing details about backend services.

core/src/types/operator/operator.rs

+                  /// if `version` is not set, or is set to false, only the active files with the current version will be returned.
+                  ///
+                  /// when `version` is set to true and the backend service has `versioning` enabled, all object versions will be returned.
+                  /// if `versioning` is not enabled, an error with kind [`ErrorKind::Unsupported`] will be returned.

Member

Xuanwo Sep 27, 2024

I will discuss this at #5145. Generally, I don't want to declare it as part of our public API (or expected behavior).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/docs