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

feat: Request-ID plugin add snowflake algorithm #4559

Merged
merged 38 commits into from
Aug 9, 2021
Merged

feat: Request-ID plugin add snowflake algorithm #4559

merged 38 commits into from
Aug 9, 2021

Conversation

dickens7
Copy link
Contributor

@dickens7 dickens7 commented Jul 7, 2021

What this PR does / why we need it:

#4209

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@dickens7 dickens7 changed the title feat Request-ID plugin add snowflake algorithm feat: Request-ID plugin add snowflake algorithm Jul 7, 2021
@spacewander
Copy link
Member

Need to make the CI pass.

@dickens7
Copy link
Contributor Author

dickens7 commented Jul 8, 2021

CI help

@Yiyiyimu
Copy link
Member

Yiyiyimu commented Jul 8, 2021

CI help

Rerun to see how it goes


Good to go

@Yiyiyimu
Copy link
Member

Yiyiyimu commented Jul 8, 2021

Hi @dickens7 do you mind also add documentations of this feature

apisix/plugins/request-id.lua Outdated Show resolved Hide resolved
apisix/plugins/request-id.lua Outdated Show resolved Hide resolved
apisix/plugins/request-id.lua Outdated Show resolved Hide resolved
apisix/plugins/request-id.lua Outdated Show resolved Hide resolved
apisix/plugins/request-id.lua Outdated Show resolved Hide resolved
apisix/plugins/request-id.lua Show resolved Hide resolved
apisix/plugins/request-id.lua Show resolved Hide resolved
apisix/plugins/request-id.lua Outdated Show resolved Hide resolved
@dickens7
Copy link
Contributor Author

dickens7 commented Jul 14, 2021

TODO: upgrade api7/lua-snowflake to 2.1-1

docs/zh/latest/plugins/request-id.md Outdated Show resolved Hide resolved
apisix/plugins/request-id.lua Show resolved Hide resolved
end
return snowflake:next_id()
end

Copy link
Member

Choose a reason for hiding this comment

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

Could we put them under utils? Snowflake ID generating should also be used elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's implement this in the next pr

@spacewander
Copy link
Member

Please solve the conflict.

docs/en/latest/plugins/request-id.md Outdated Show resolved Hide resolved
docs/zh/latest/plugins/request-id.md Outdated Show resolved Hide resolved
apisix/plugins/request-id.lua Outdated Show resolved Hide resolved

| Name | Type | Requirement | Default | Valid | Description |
| ------------------- | ------- | ------------- | -------------- | ------- | ------------------------------ |
| enable | boolean | required | false | | When set it to true, enable the snowflake algorithm. |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| enable | boolean | required | false | | When set it to true, enable the snowflake algorithm. |
| enable | boolean | optional | false | | When set it to true, enable the snowflake algorithm. |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified jsonSchema required = {"enable", "snowflake_epoc "}

Copy link
Member

Choose a reason for hiding this comment

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

Once you provide a default value, the field is actually optional, as users don't need to specify their values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I'll make it optional

docs/en/latest/plugins/request-id.md Outdated Show resolved Hide resolved
apisix/plugins/request-id.lua Show resolved Hide resolved
goto continue
end

local _, err1 = etcd_cli:setnx(prefix .. tostring(id), uuid)
Copy link
Contributor

Choose a reason for hiding this comment

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

So we have to write ETCD so that we can write the uuid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of writing etcd here is to generate DataMachineId for snowflake. The value is uuid because etcd_cli:setnx still returns succeeded even if the key exists. UUID is used for verification after etcd_cli:setnx.

snowflake:
enable: false
snowflake_epoc: 1609459200000 # the starting timestamp is expressed in milliseconds
data_machine_bits: 12 # data machine bit, maximum 31, because Lua cannot do bit operations greater than 31
Copy link
Member

Choose a reason for hiding this comment

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

should check data_machine_bits + sequence_bits = 22 always? may the test case need to cover it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

data_machine_bits and sequence_bits are not fixed and can be configured according to different requirements


- `snowflake_epoc` default start time is `2021-01-01T00:00:00Z`, and it can support `69 year` approximately to `2090-09-0715:47:35Z` according to the default configuration
- `data_machine_bits` corresponds to the set of workIDs and datacEnteridd in the snowflake definition. The plug-in aslocates a unique ID to each process. Maximum number of supported processes is `pow(2, data_machine_bits)`. The default number of `12 bits` is up to `4096`.
- `sequence_bits` defaults to `10 bits` and each process generates up to `1024` ID per second
Copy link
Member

Choose a reason for hiding this comment

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

The per second here and the per millisecond above confuse me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the definition of two parts of the snowflake algorithm. Millisecond is the definition of the timestamp part, which means that the timestamp part is in milliseconds. Second is the definition of the sequence number part, which means that the number of id can be generated per second

ngx.say("ids not unique")
return
end
ids[id] = true
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to check that the length of the ids has reached a fixed value? such as 100, or at least 2. I am worried that there is no data in the ids due to sync.

@tzssangglass
Copy link
Member

LGTM

@spacewander spacewander merged commit e127cc7 into apache:master Aug 9, 2021
@@ -53,7 +221,6 @@ function _M.rewrite(conf, ctx)
end
end


Copy link
Contributor

Choose a reason for hiding this comment

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

why remove this blank?

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.

9 participants