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

feature: metasrv has to be compatible with 20220413-34e89c99e4e35632718e9227f6549b3090eb0fb9 #4901

Merged
merged 2 commits into from
Apr 18, 2022

Conversation

drmingdrmer
Copy link
Member

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

feature: metasrv has to be compatible with 20220413-34e89c99e4e35632718e9227f6549b3090eb0fb9

Let metasvr be able to load the log-entry format before commit
34e89c9,
in which the Cmd format changed.

To solve th compatible issue, an intermedia type is introduced, which is
a super set of the old and new type.
When loading a record, first load it into the superset type, then reduce
to the latest type.

Changelog

  • New Feature

Related Issues

@vercel
Copy link

vercel bot commented Apr 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
databend ⬜️ Ignored (Inspect) Apr 18, 2022 at 9:15AM (UTC)

@mergify
Copy link
Contributor

mergify bot commented Apr 17, 2022

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@mergify mergify bot added the pr-feature this PR introduces a new feature to the codebase label Apr 17, 2022
@drmingdrmer drmingdrmer marked this pull request as ready for review April 17, 2022 16:42
@BohuTANG
Copy link
Member

BohuTANG commented Apr 17, 2022

Is it possible set a version for the metadata, default is version_0, if the metadata changed upgrade it to version_1, and we handle the versions?
This time we can force the user to upgrade or removed the if-exists from the metadata, next time will compatibly with the old version.

@lichuang
Copy link
Contributor

i wonder if there is more elegant way to do compatible in Rust enum type, like protobuf optional type?

In this PR's way, the next time Cmd upgrade, it will has a new cmd_xxxx type to compatible, it is boring....

@lichuang
Copy link
Contributor

i wonder if there is more elegant way to do compatible in Rust enum type, like protobuf optional type?

In this PR's way, the next time Cmd upgrade, it will has a new cmd_xxxx type to compatible, it is boring....

or, IMHO, if the CMD type may be upgrade in the future and it is the key data of meta API, we can define it as a protobuf type, to make it compatible easily?

@flaneur2020
Copy link
Member

how about reduce the business logics like CreateDatabase / CreateShare into the raft command, instead make them depends on some generic K/V operations like PutWriteBatch / CommitTx?

this may reduce the possbility of changes about low level raft commands

@lichuang
Copy link
Contributor

how about reduce the business logics like CreateDatabase / CreateShare into the raft command, instead make them depends on some generic K/V operations like PutWriteBatch / CommitTx?

this may reduce the possbility of changes about low level raft commands

emm, good idea.

@drmingdrmer drmingdrmer marked this pull request as draft April 18, 2022 03:53
@drmingdrmer
Copy link
Member Author

Is it possible set a version for the metadata, default is version_0, if the metadata changed upgrade it to version_1, and we handle the versions? This time we can force the user to upgrade or removed the if-exists from the metadata, next time will compatibly with the old version.

Right. It is the second part of the plan.
Next time to upgrade, we need to:

  1. detect the version changes, as you mentioned above,
  2. and load the old version data then convert them to the new version. This is what this PR does.

@drmingdrmer
Copy link
Member Author

i wonder if there is more elegant way to do compatible in Rust enum type, like protobuf optional type?

The compatible intermediate struct cmd_20220413.Cmd loads the data in this way,
and convert to a struct without Option fields.

Using Option directly in the latest Cmd is not considered because it may confuse the user by implying that a field is optional.

In this PR's way, the next time Cmd upgrade, it will has a new cmd_xxxx type to compatible, it is boring....

It does not have to be, if we can limit the oldest version that is backward compatible with, e.g.: the publish metasvr version could be:

  • version A;
  • version B that is compatible with A
  • version B that is NOT compatible with A
  • version C that is compatible with B
  • ...

And there probably is not another cmd_xxx, to be compatible with two older version, the max compatible Cmd has to be able to load both old version A and old version B.
Thus the max compatible Cmd will just become a bit larger...

@drmingdrmer
Copy link
Member Author

or, IMHO, if the CMD type may be upgrade in the future and it is the key data of meta API, we can define it as a protobuf type, to make it compatible easily?

Compatibility is not an issue in such a case. Actually, rust serde provides more flexible ser/de ability. Such as Field attributes

The dirty job is how to avoid using Option in a struct, to prevent abuse of unwrap() etc.

@drmingdrmer
Copy link
Member Author

how about reduce the business logics like CreateDatabase / CreateShare into the raft command, instead make them depends on some generic K/V operations like PutWriteBatch / CommitTx?

this may reduce the possbility of changes about low level raft commands

This solution is considered before but we were facing the fact that CreatDatabase and else are not pure batch write but conditional updates.
It requires the metasrv to provide a small language with which a complete tx can be expressed as this demo illustrated(current solution and the tx-language solution):

fn create_table() {
    let xx = dbs.get((args.db_name, args.table_name));
    if xx.is_ok() {
        return Ok();
    } else {
        let id = new_id();
        dbs.insert((args.db_name, args.table_name), id);
        tables.insert((args.db_name, id), args.table);
        return Ok();
    }
}
pub enum Cmd {

    /// Conditional evaluate: 
    /// - first evaluate `condition`, it has to be a bool value.
    /// - then evaluate then_cmd or else_cmd
    Cmd::IfThenElse {
        condition: Cmd,
        then_cmd: Cmd,
        else_cmd: Cmd,
    }

    /// Assign a Cmd and save the output into a temporary variable with name `var`.
    ///
    /// 
    Cmd::Assign {
        var: String, 
        cmd: Cmd
    }

    /// A series of Cmd that will be evaluated one by one.
    Cmd::Statements(Vec<Cmd>),

    /// Return true if a variable is `None`.
    Cmd::IsNull(String), 

    /// Return the value of a variable, the value type is `AppliedState`.
    Cmd::Var(String), 


    /// Generate a new unique id in a specified name space.
    Cmd::NewId(String), 

    // Tree operations

    /// alias of BTreeMap.get()
    ///
    Cmd::GetKV{
        /// The tree to acces.
        /// Only predefined trees are provided, such as:
        /// table, db, 
        tree: String, 
        key: impl IntoTreeKey, 
    }
}

///

Statements(vec![

    Assign {
        var: "curr",
        cmd: GetKV{
            tree: "db_tbl_id",
            key: (args.db_name, args.table_name)
        }
    },
    IfThenElse {
        condition: IsNull("curr"),
        then_cmd: Var("curr"),
        else_cmd: Statements(vec![
            Assign {
                var: "id",
                cmd: NewId("table"),
            },
            Upsert {
                tree: "db_tbl_id",
                key: (args.db_name, args.table_name),
                value: Var("id"),
            },
            Upsert {
                tree: "db_tbl_t",
                key: (args.db_name, Var("id")),
                value: args.table,
            },
            Var(id)
        ])
    }

])

@drmingdrmer
Copy link
Member Author

Converted to draft because several test should be added before merge.:(

Let metasvr be able to load the log-entry format before commit
34e89c9,
in which the `Cmd` format changed.

To solve th compatible issue, an intermedia type is introduced, which is
a super set of the old and new type.
When loading a record, first load it into the superset type, then reduce
to the latest type.

- fix: databendlabs#4890
@drmingdrmer drmingdrmer marked this pull request as ready for review April 18, 2022 09:04
@drmingdrmer drmingdrmer added A-meta Area: databend meta serive C-bug Category: something isn't working rust Pull requests that update Rust code labels Apr 18, 2022
@drmingdrmer
Copy link
Member Author

@lichuang @ariesdevil anything need to improve?

Copy link
Contributor

@ariesdevil ariesdevil left a comment

Choose a reason for hiding this comment

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

lgtm

@BohuTANG BohuTANG merged commit 50c6e3e into databendlabs:main Apr 18, 2022
@drmingdrmer drmingdrmer deleted the fix-upgrade branch April 18, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: databend meta serive C-bug Category: something isn't working need-review pr-feature this PR introduces a new feature to the codebase rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: metadata error
6 participants