Skip to content

no fee based aggregation/batching #492

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

no fee based aggregation/batching #492

wants to merge 2 commits into from

Conversation

LexLuthr
Copy link
Contributor

@LexLuthr LexLuthr commented Apr 22, 2025

fixes #489

Should not break nodes running older code during upgrade

  • Test in devnet

@LexLuthr LexLuthr requested review from snadrus and magik6k April 22, 2025 14:18
Copy link
Contributor

@snadrus snadrus left a comment

Choose a reason for hiding this comment

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

fancy! I'd like Magik to check the SQL logic

@LexLuthr
Copy link
Contributor Author

fancy! I'd like Magik to check the SQL logic

It is basically the same function but we don't check for fee anymore.

Copy link
Collaborator

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Just the question about the config stuff. The rest looks good / makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do the changes here do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Those are mostly due to how comp library works. It treats "0h5m10s" different from "5h0m0s" for some reason.
The other change is because if we have 2 config parameters with same name then current load confuse the value because map we created was global. This change updates the map keys to be config section specific. To explain is simply:
The preCommit msg timeout of 1 hour (default 4 hour) will no longer be treated as default value due to commit msg timeout default being 1 hour.

*/

FOR batch_rec IN
WITH locked AS (
Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding is that WITH locked AS does nothing at all in yugabyte

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are confusing the table name with "FOR UPDATE;" at the end for actual locking. I can change the table name to avoid confusing.

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.

Default to use aggregation with prove commits to save significant gas usage
3 participants