Log file of current hour gets overwritten by default

[abitmore]

Steps to reproduce:

• change config.ini so default log will be written to file

# declare an appender named "default" that writes messages to default.log
[log.file_appender.default]
filename=logs/default/default.log
# filename can be absolute or relative to this config file

# route any messages logged to the default logger to the "stderr" logger and "default" logger we
# declared above, if they are info level are higher
[logger.default]
level=info
appenders=stderr,default

• start witness_node
• stop witness_node
• check log file (witness_node_data_dir/logs/default/default.log)
• start witness_node again
• check log file (witness_node_data_dir/logs/default/default.log)

The log file will be overwritten on restart.

IMHO the default behavior should be appending but not overwriting.

CORE TEAM TASK LIST

• Evaluate / Prioritize Feature Request
• Refine User Stories / Requirements
• Define Test Cases
• Design / Develop Solution
  • Issue estimation: 2 hours
  • Issue assigned to @nanomobile
• Perform QA/Testing
• Update Documentation

[clockworkgr]

Not completely confident in C++ coding yet to simply do a PR, but since I wanna start getting involved, I just wanna run this by you.

It seems that here:
https://github.com/bitshares/bitshares-fc/blob/master/src/log/file_appender.cpp#L148

The file is opened , passing a bitmask for write & append mode.

However, here:
https://github.com/bitshares/bitshares-fc/blob/master/src/io/fstream.cpp#L32

It seems like the bitmask passed is ignored and opening in binary mode is hardcoded in instead.

I assume the correct way of tackling this is to fix fstream.cpp to use the bitmask passed and visit all calls to open() to make sure that binary mode is added to the bitmask.

Am I correct in any of this?

[nanomobile]

I want to claim this issue, help me guys please to make correct workflow for first time

[ryanRfox]

@nanomobile has claimed this Issue. I've moved it to the Community Claims project board. The OP provides hints for a Test Case. Also, @clockworkgr provides hints above for potential fixes within fstream.cpp and confirmed by @abitmore . Please begin your local development with a checkout from the develop branch. Please rebase your local feature branch prior to creating your PR to develop.

[nanomobile]

how much hours is reserved for this issue ?

[nanomobile]

What is your estimates on this issue so the Community know how many hours it is expected to take ?

[ryanRfox]

@nanomobile Checking with Core Team devs for assistance with estimation...

[nanomobile]

Thanks a lot ! Have a perfect day !

[oxarbitrage]

i am not very good in estimations but i will say it will take 1 hour to someone with the needed knowledge of git and bitshares. clockwork already pointed to some code where it can be fixed. it could take some more(2 hours) to someone not that familiar with any of this but i think not much more than that in this particular case.

the estimations in general i think can be done with a floor of no less than 1 hour and a variable roof in case something is more complicated than initially estimated or reviewers request changes to the code, etc)

[nanomobile]

while debugging I found that problem isn't located as clockwork wrote.

[clockworkgr]

Yeah, don't take my comment as gospel...my c++ knowledge is rudimentary at best...I just wanted to know if I was thinking across the right lines

[nanomobile]

yes sure ! Understood ! I thought the same as you ;-) Just checked your version :-) I thought you're right and I can fix it less than 1 hour :-) but need to research & debug more ...

[oxarbitrage]

can be extended to 2 hours without problems IMO.

[jmjatlanta]

I'd say 1 1/2 to 2hr is fair.

** Warning... Soapbox **
While some may balk at the time it takes to handle a task, please remember that often this must include research, estimation, consultation, implementation, testing, and documentation.

I'm not saying this task requires all these steps. But coding time is only part of the equation. Often, we attack a problem only to find that it affects something else. Or find that if we do it the right way, we also have to adjust this "other thing over there".

My $0.02 (includes the $0.02 VAT), is that the potential candidate should have a clear understanding of what is expected, and can assist in the estimation. The first few attempts are a learning experience, and often cost the developer. But the rewards are there for endurance (speaking from experience on this point).

If tasks grow beyond the initial requirements (this happens more often than we want), we should reward the developer for the value of what they were able to accomplish, even if the results are only documentation of a deeper problem.
** stepping down from soapbox **

[abitmore]

I've mentioned my opinions above, quoting here:

• change the type of the 2nd parameter to std::ios_base::openmode mode, and
  • change the default value to std::ios_base::out | std::ios_base::binary
  • if the 2nd parameter is set, open the file with std::ios_base::out | std::ios_base::binary | the_given_value

[jmjatlanta]

I'd say @abitmore has a good solution. That would require the minimal amount of changes to source code and fix the current problem. I think a separate issue should be created to refactor fc::fstream so as to be consistent with standard fstream expectations (i.e. text mode). I will create that issue.

[nanomobile]

so @abitmore 's solution is the best ? right ? Should I implement it ? does anybody disagree ?

[nanomobile]

@abitmore yes I mean ifstream has binary issue, the same situation means that there is issue too with ifstream :-)

[nanomobile]

new changes in PR are ready
bitshares/bitshares-fc#56

bitshares/bitshares-fc@71b6f15

@abitmore @oxarbitrage @jmjatlanta @ryanRfox @clockworkgr so we can close this issue and merge changes from this PR ?

[abitmore]

Fixed with #1104.