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

[mainnet] New defaults for max- and partial-ops #415

Merged
merged 1 commit into from Oct 13, 2017

Conversation

xeroc
Copy link
Member

@xeroc xeroc commented Oct 10, 2017

The reasoning here is that maintainers and operators don't need to
customize the configuration fiel in order to have the BitShares backend
running in low memory mode. The number 1000 should be sufficient for
most operations (even exchanges) to include all needed operations in the
exchanges' account.

@oxarbitrage
Copy link
Member

i like it. i am wondering if we should add a msg in the log that node is starting with this setup or add somewhere in the documentation that this is a reduced node. what do you think?

@abitmore
Copy link
Member

I'd highly recommend NOT change the default behavior. Exchange accounts have far more than 1K records. Any changes may break existing business.

@xeroc
Copy link
Member Author

xeroc commented Oct 10, 2017

How about this comes with a warning in the release notes that says that exchanges should set the parameters to their liking and or even set track-account?

@pmconrad
Copy link
Contributor

I agree with @abitmore - changing default behaviour is a bad idea and is likely to break existing installations.
I wouldn't mind changing the default config file though, that should affect only new installations.

@oxarbitrage
Copy link
Member

i understand both sides. existing business vs potential business lost. existing business is always more important. what do you think about the default config file change @abitmore ?

@abitmore
Copy link
Member

I remember the idea about changing default config values has been discussed somewhere, perhaps in the Telegram channel. It's true that it will bring less trouble (but not zero trouble), so I think it's perhaps OK.

@xeroc xeroc force-pushed the mainnet-new-defaults-partial-max-ops branch from 8d4123a to 9be9015 Compare October 13, 2017 08:56
…iles

The reasoning here is that maintainers and operators don't need to
customize the configuration file - if they never used bitshares-core,
while it keeps integrity if a config.ini already exists - in order to
have the BitShares backend running in low memory mode. The number 1000
should be sufficient for most operations (even exchanges) to include all
needed operations in the exchanges' account.
@xeroc xeroc force-pushed the mainnet-new-defaults-partial-max-ops branch from 9be9015 to 7456e2e Compare October 13, 2017 08:57
@xeroc
Copy link
Member Author

xeroc commented Oct 13, 2017

@pmconrad I force pushed a different fix for this issue that stores the stuff in file (if created newly).

Please consider adding this to the new hardfork release right away.

I did testing, it seems to be working as desired.

@oxarbitrage If you don't mind, please also do some testing

@pmconrad
Copy link
Contributor

I'm not a friend of last minute changes, but if Alfredo approves I'm OK with it.

@oxarbitrage oxarbitrage self-requested a review October 13, 2017 12:13
Copy link
Member

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

i like the implementation and it also works good. i think we can also mention this change in the README. merging it, nice job.
in regards to last minute changes to the release let's add this one if possible but no more and start thinking on a next release. Releases that don't have a hardfork should be easier to make and we can make them more often with enough or significant changes from develop to master.

@oxarbitrage oxarbitrage merged commit 952395d into develop Oct 13, 2017
@pmconrad
Copy link
Contributor

After testing I have found that this patch has several downsides:

  • the options end up twice in the generated config file, which is confusing
  • the new documentation is wrong, it mentions the wrong defaults
  • the new values are written to the config file, but they get active only after a restart (!)

@oxarbitrage
Copy link
Member

in regards to point 1 i used the pull and the new features didn't ended up twice but only once in the created config file.
in point 2 what is the documentation you are referring to ? the pull only change 1 file and the documentation in there seems correct to me.
i didn't realized of point 3 when testing but if this is the case and a restart is needed then i think it will need to be fixed.

@pmconrad
Copy link
Contributor

This is what was already there:

# Keep only those operations in memory that are related to account history tracking
# partial-operations = 

# Maximum number of operations per account will be kept in memory
# max-ops-per-account = 

...and this is new:

# Keep only those operations in memory that are related to account history tracking (defaults to true)
partial-operations = true

# Maximum number of operations per account will be kept in memory (defaults to 1000)
max-ops-per-account = 1000

Both mentioned defaults are wrong, partial-operations defaults to false and max ops defaults to infinite.

@oxarbitrage
Copy link
Member

you are right , i didn't saw it as it was commented, sorry about that. i reverted the merge until this problems are addressed properly.

@pmconrad pmconrad mentioned this pull request Oct 14, 2017
@abitmore abitmore deleted the mainnet-new-defaults-partial-max-ops branch January 13, 2018 18:41
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.

None yet

4 participants