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

Log file of current hour gets overwritten by default #809 #56

Merged
merged 2 commits into from Jul 25, 2018

Conversation

5 participants
@nanomobile
Copy link

commented Jun 29, 2018

@nanomobile nanomobile referenced this pull request Jun 29, 2018

Closed

Log file of current hour gets overwritten by default #809

4 of 8 tasks complete

@nanomobile nanomobile force-pushed the nanomobile:valera_issue_809 branch from 8a0963e to 153f568 Jun 29, 2018

create_hard_link(log_filename, link_filename);
out << rez;

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jun 29, 2018

I believe this new code is taking what is in "mylog.log" and copying it to "mylog.log.datetime" and leaving "mylog.log.datetime" open for writing.

That may be what we want for reboots, but is not what we want for normal rollovers. On a rollover, the file will continue to grow ever larger, as it has the previous rotation_interval messages in it as well.

My suggestion (I believe this is how it works in old log4j code):

  • Always open mylog.log with the append flag. Always write log messages to mylog.log
  • On a rollover, rename mylog.log to mylog.log.datetime, and create a new mylog.log, assigning it to "out" for future messages to write to it.

Depending on how the rename is done (not sure how boost implements this), this can prevent the windows problem, as a rename does not require the file to be closed. If any other processes are writing to it, they should be okay.

Or, if you would rather continue with the existing code, re-add the if statement and only copy the previous contents if fc::exists. This may be the safer option, as my solution mentioned above is dated and direct from (often faulty) memory chips in my head.

Update: Ignore my above comment for a bit. I think I misinterpreted the code...
Update2: No, I still think I'm right. As it is written, rollovers will take anything in the current log file and copy it to the subsequent file.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jun 29, 2018

Author

@jmjatlanta
where should I add if fc::exists statement and for which file to check ? clarify a bit more please ...

Thanks a lot !

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jun 29, 2018

Don't take my word for it. Test to make sure what I'm saying is true. But perhaps something like:

if( fc::exists( log_filename ) )
{
   out.open( log_filename, std::ios_base::out | std::ios_base::app );
   out << rez;
}
else
   out.open( log_filename, std::ios_base::out | std::ios_base::app);

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jun 29, 2018

Author

@jmjatlanta - look please now the snippet of code is next:

           string rez;
           read_file_contents(link_filename, rez);
           remove_all(link_filename);  // on windows, you can't delete the link while the underlying file is opened for writing
           out.open( log_filename, std::ios_base::out | std::ios_base::app );
           create_hard_link(log_filename, link_filename);
           if (fc::exists( log_filename ))
               out << rez;

just added fc::exists( log_filename ) because out.open( log_filename, std::ios_base::out | std::ios_base::app ) this line of code will be executed in both true/false statements - it doesn't depend on fc::exists rezult.

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jun 29, 2018

The if would need to replace the first out.open above. The pseudocode is:

  • If (the file mylog.log.201806291800 exists)
    • then open it and copy what was in mylog.log into it.
  • Otherwise, open the file mylog.log.201806291800

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jun 29, 2018

Perhaps it would be good to write some test cases. That way we know we're fixing the problem. Let me see if I can put one together quickly...

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jun 29, 2018

Author

@jmjatlanta Thanks a lot ! I'm checking and thinking about test cases too ;-)

@nanomobile nanomobile force-pushed the nanomobile:valera_issue_809 branch from 153f568 to fc5dea6 Jun 29, 2018

@ryanRfox ryanRfox referenced this pull request Jun 29, 2018

Closed

fixed #809 issue, open log file in append mode #1102

0 of 1 task complete
@jmjatlanta
Copy link

left a comment

Well, it took way too long, but here is some changes that will allow you to test your changes...

9461639

If you make those changes in your branch, you should be able to run the test, and then go to your /tmp directory and view the results.

./tests/all_tests --run_test=logging_tests/log_reboot --log_level=message
is what I use.

Note: The test gives a SIGABRT due to the cruel way I set up the test. But the test should still run. Let me know if you have any problems.

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jun 30, 2018

@jmjatlanta Thank you very much ! Very nice ! I'm checking now ...

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jun 30, 2018

@jmjatlanta your logging_test.cpp works well enough ! Should I commit it in Pull Request too ?

@nanomobile nanomobile force-pushed the nanomobile:valera_issue_809 branch from fc5dea6 to 3b39aa1 Jun 30, 2018

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jun 30, 2018

@jmjatlanta I've added your test in commit too :-)

@@ -82,13 +82,13 @@ namespace fc {
out.flush();
out.close();
}
string rez;
read_file_contents(link_filename, rez);

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 1, 2018

Member

I don't think this is the correct solution. Log files can be very very large, for example, hundreds of Gigabytes.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 1, 2018

Author

@abitmore what exactly do you mean ? read_file_contents isn't a good solution ?

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 1, 2018

Member

Correct.

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 1, 2018

Member

Imagine that size of current log file is 100GB, of course we don't want to read the entire content to a string, then write the string to another file. Open the file in "append" mode is the correct approach.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 1, 2018

Author

ok sure understood, thanks a lot ! 👍

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 2, 2018

Both files (the timestamp and non-timestamp file) were the same size, with the same information. Old entries were not there.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 2, 2018

Author

let me check again original code from develop branch

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 2, 2018

Author

so if this is broken then I should just fix it without copying file and without making huge files right ?

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 2, 2018

That would be great, and would solve the issue.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 2, 2018

Author

👍 Now I understood all things clearly enough and now I know what exactly we expect as a best quality result for this issue ! Thanks a lot for all your efforts !!! 👍

@@ -155,6 +155,22 @@ namespace fc {
}
}

file_appender::file_appender( const config& arg ) : fc::appender(),
my( new impl( arg ) )

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 1, 2018

Member

Where in the code used this constructor except the new test case?

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 1, 2018

Author

this test case was made by @jmjatlanta , maybe he can help to answer on your question

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 1, 2018

Author

I think nowhere in the code this constructor was used except current test case, so @abitmore do you mean we don't need it ?

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 2, 2018

Sorry guys, I added that constructor as a cruel way to construct a file_appender. I cannot create an fc::variant as it will not compile. Rather than fight that battle, I threw in a new constructor. We now need to fight the battle and win, and then we can eliminate this constructor.

Perhaps I can look at it again with fresh eyes to spot where the problem is. I'll update my results here.

Compilation issue: If I attempt to create an fc::variant passing in an fc::file_appender_config, I get:
/home/jmjatlanta/Development/cpp/bitshares-fc/include/fc/variant.hpp: In instantiation of ‘fc::variant::variant(const T&, uint32_t) [with T = fc::file_appender::config; uint32_t = unsigned int]’:
/home/jmjatlanta/Development/cpp/bitshares-fc/tests/logging_tests.cpp:33:33: required from here
/home/jmjatlanta/Development/cpp/bitshares-fc/include/fc/variant.hpp:564:17: error: no matching function for call to ‘to_variant(const fc::file_appender::config&, fc::variant&, uint32_t&)’
to_variant( val, *this, max_depth );

I know this should work, as we are doing it when we start the witness_node. But it doesn't work here, and I have yet to find out why.

Another hack in here is that the test crashes due to reference counting done by some underlying object. Programs such as witness_node fire up the whole logging framework which avoids this problem, but makes testing just this appender difficult. Such are the pains of building unit tests for existing code. :(

Update I found the problem with fc::variant. I needed to include fc/reflect/variant.hpp instead of fc/variant.hpp. Still working on other modifications

This comment has been minimized.

Copy link
@nanomobile

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 2, 2018

Fixed the variant issue. See this commit: 21cc744

This comment has been minimized.

Copy link
@nanomobile

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 2, 2018

And now fixed the SIGSEGV in the test. See this commit: a549c6e

Note: Please ignore the comment lines at the top, I moved some stuff and forgot to delete the comment.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 2, 2018

Author

👍 Thank you very much ! Great job done !

@nanomobile nanomobile force-pushed the nanomobile:valera_issue_809 branch 2 times, most recently from aab6263 to 237e8e7 Jul 2, 2018

@@ -82,13 +82,16 @@ namespace fc {
out.flush();
out.close();
}
string rez;
read_file_contents(link_filename, rez);

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 2, 2018

Author

The default behaviour is fixed now, I've checked it. Test please too on your side. Awaiting your results ...

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 2, 2018

Member

The code still reads the contents of a file to a string? It need to be fixed.

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 2, 2018

Member

You forgot to push your new code?

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 2, 2018

Author

no, I didn't forgot to push my new code :-) read_file_contents copy just piece of data logs which depends on rotation intervals from config file. In current solution as you can see we won't have huge 100 GB log files or even just a few GB files won't be, depends also on file rotation intervals from config.

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 2, 2018

Member

What if the rotation interval is configured to a day?

bool flag = false;
if (fc::exists( log_filename ))
flag = true;
out.open( log_filename, std::ios_base::out | std::ios_base::app );

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 2, 2018

Member

I think the actual bug is inside out.open(...), it is called here with std::ios_base::app, technically it should open the file in append mode, but it actually opened the file in overwrite mode.

(same as @clockworkgr's comment here: bitshares/bitshares-core#809 (comment))

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 4, 2018

Author

after making next changes in fstream.cpp file
void ofstream::open( const fc::path& file, int m ) {
const boost::filesystem::path& bfp = file;
my->ofs.open( bfp, (std::ios_base::openmode)m | std::ios::binary );
}

I see next compile errors, can anybody help me please ? @abitmore @oxarbitrage @jmjatlanta @ryanRfox @clockworkgr , I think there is also another issue, what do you think guys ?

[ 73%] Generating egenesis_brief.cpp, egenesis_full.cpp
embed_genesis: Reading genesis from file /home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/genesis.json

embed_genesis: parsing tmplsub parameter "/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_brief.cpp.tmpl---/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_brief.cpp"
embed_genesis: parsing tmplsub parameter "/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp.tmpl---/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp"
Scanning dependencies of target graphene_egenesis_full
[ 74%] Building CXX object libraries/egenesis/CMakeFiles/graphene_egenesis_full.dir/egenesis_full.cpp.o
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:941870:50: error: redefinition of ‘const char graphene::egenesis::genesis_json_array [941747][41]’
static const char genesis_json_array[941747][40+1] =
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:49:19: note: ‘const char graphene::egenesis::genesis_json_array [941747][41]’ previously defined here
static const char genesis_json_array[941747][40+1] =
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp: In function ‘graphene::chain::chain_id_type graphene::egenesis::get_egenesis_chain_id()’:
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:1883621:15: error: redefinition of ‘graphene::chain::chain_id_type graphene::egenesis::get_egenesis_chain_id()’
chain_id_type get_egenesis_chain_id()
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:941800:15: note: ‘graphene::chain::chain_id_type graphene::egenesis::get_egenesis_chain_id()’ previously defined here
chain_id_type get_egenesis_chain_id()
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp: In function ‘void graphene::egenesis::compute_egenesis_json(std::__cxx11::string&)’:
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:1883626:6: error: redefinition of ‘void graphene::egenesis::compute_egenesis_json(std::__cxx11::string&)’
void compute_egenesis_json( std::string& result )
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:941805:6: note: ‘void graphene::egenesis::compute_egenesis_json(std::__cxx11::string&)’ previously defined here
void compute_egenesis_json( std::string& result )
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp: In function ‘fc::sha256 graphene::egenesis::get_egenesis_json_hash()’:
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:1883637:12: error: redefinition of ‘fc::sha256 graphene::egenesis::get_egenesis_json_hash()’
fc::sha256 get_egenesis_json_hash()
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:941816:12: note: ‘fc::sha256 graphene::egenesis::get_egenesis_json_hash()’ previously defined here
fc::sha256 get_egenesis_json_hash()
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp: At global scope:
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:1883691:50: error: redefinition of ‘const char graphene::egenesis::genesis_json_array [941747][41]’
static const char genesis_json_array[941747][40+1] =
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:49:19: note: ‘const char graphene::egenesis::genesis_json_array [941747][41]’ previously defined here
static const char genesis_json_array[941747][40+1] =
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp: In function ‘graphene::chain::chain_id_type graphene::egenesis::get_egenesis_chain_id()’:
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:2825442:15: error: redefinition of ‘graphene::chain::chain_id_type graphene::egenesis::get_egenesis_chain_id()’
chain_id_type get_egenesis_chain_id()
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:941800:15: note: ‘graphene::chain::chain_id_type graphene::egenesis::get_egenesis_chain_id()’ previously defined here
chain_id_type get_egenesis_chain_id()
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp: In function ‘void graphene::egenesis::compute_egenesis_json(std::__cxx11::string&)’:
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:2825447:6: error: redefinition of ‘void graphene::egenesis::compute_egenesis_json(std::__cxx11::string&)’
void compute_egenesis_json( std::string& result )
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:941805:6: note: ‘void graphene::egenesis::compute_egenesis_json(std::__cxx11::string&)’ previously defined here
void compute_egenesis_json( std::string& result )
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp: In function ‘fc::sha256 graphene::egenesis::get_egenesis_json_hash()’:
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:2825458:12: error: redefinition of ‘fc::sha256 graphene::egenesis::get_egenesis_json_hash()’
fc::sha256 get_egenesis_json_hash()
^
/home/valera/Projects/Blockchain/Graphene/BitShares/bitshares-core/libraries/egenesis/egenesis_full.cpp:941816:12: note: ‘fc::sha256 graphene::egenesis::get_egenesis_json_hash()’ previously defined here
fc::sha256 get_egenesis_json_hash()
^
libraries/egenesis/CMakeFiles/graphene_egenesis_full.dir/build.make:72: recipe for target 'libraries/egenesis/CMakeFiles/graphene_egenesis_full.dir/egenesis_full.cpp.o' failed
make[2]: *** [libraries/egenesis/CMakeFiles/graphene_egenesis_full.dir/egenesis_full.cpp.o] Error 1
CMakeFiles/Makefile2:929: recipe for target 'libraries/egenesis/CMakeFiles/graphene_egenesis_full.dir/all' failed
make[1]: *** [libraries/egenesis/CMakeFiles/graphene_egenesis_full.dir/all] Error 2
Makefile:127: recipe for target 'all' failed
make: *** [all] Error 2

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 4, 2018

Member

I don't think the compiler error is related to the change.

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 4, 2018

Member

Oh I guess I was wrong. It can be related. The temporary/intermediate genesis file are opened with append mode. Need to fix it.

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 4, 2018

Member

The compiler error is due to this code:

      fc::ofstream outfile( dest_filename );
      outfile.write( out_str.c_str(), out_str.size() );
      outfile.close();

It turns out that the 2nd parameter int m of ofsteam::open() is incompatible with std::ios_base::openmode, see the definition here:

enum mode { out, binary };
ofstream();
ofstream( const fc::path& file, int m = binary );
~ofstream();
void open( const fc::path& file, int m = binary );

This is horrible coding practice, it may have been impacting a lot of code elsewhere, please be extremely careful. We may need to write new test cases for all of them.

@pmconrad @jmjatlanta please take a look if have time.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 4, 2018

Author

should I fix the temporary/intermediate genesis file to be opened with binary mode ? right ? or who should fix it ? can I do it ? if YES, can you help me please to find where are those code lines in which files ?

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 4, 2018

Author

void ofstream::open( const fc::path& file, int m ) {
const boost::filesystem::path& bfp = file;
my->ofs.open( bfp, (std::ios_base::openmode)m );
}

Now this code compiled fully and without any errors, strange :-) cannot understand the mystics, researching more and more deeply code ... I'm trying to fix this issue with the best quality and with only required minimal amount of changes !

This comment has been minimized.

Copy link
@abitmore

@nanomobile nanomobile force-pushed the nanomobile:valera_issue_809 branch 2 times, most recently from 1a857b9 to 71b6f15 Jul 6, 2018

const boost::filesystem::path& bfp = file;
my->ofs.open( bfp, std::ios::binary );
my->ofs.open( bfp, m );

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 6, 2018

Note the second point in Abit's suggestion. This will help with backward compatibility...

  • 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

So if the passed in value does not include the flags out and binary, they will be added.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 6, 2018

Author

@jmjatlanta look please fstream.hpp the default values are already there

71b6f15#diff-aae0d97a407fcae9906054c8e1b9aa86R16

71b6f15#diff-aae0d97a407fcae9906054c8e1b9aa86R19

isn't it correct ?

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 6, 2018

I believe line 32 should read something like my->ofs.open( bfp, std::ios_base::out | std::ios_base::binary | m);

That way, any existing code works just the way it used to.

This comment has been minimized.

Copy link
@nanomobile

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 6, 2018

It is almost the same, but not quite. Here is a contrived example: Think about the scenario of if someone passed only ios::ios_base::app. Without the change above, the default values get replaced with only ios::ios_base::app. The default values we provide would have been replaced.

By fixing the above, what the user sends this method gets appended to what we have, instead of replacing what we have.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 6, 2018

Author

yes you're absolutely right ! Thanks ! I've thought about that and yes we need this change in order not to overwrite our default values, I just didn't know that everywhere we need such default values std::ios_base::out | std::ios_base::binary even if we passed 2nd parameter with mode required. I thought @abitmore 's suggestion can be improved a little bit, but I was wrong, that's why still research and debug more deeply the whole BitShares Core C++ project :-)

Thanks everybody for understandings, efforts and help !

@nanomobile nanomobile force-pushed the nanomobile:valera_issue_809 branch from 71b6f15 to cdec042 Jul 6, 2018

Will review when have time

@abitmore
Copy link
Member

left a comment

Looks like the test case is checking nothing?

#include <fstream>

using namespace std;

namespace fc {
class path;
class ofstream : virtual public ostream {
public:
enum mode { out, binary };

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 6, 2018

Member

I'd recommend remove this definition.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 6, 2018

Author

@abitmore if I will remove this definition as you suggested then I will have compile errors, couldn't compile without those definitions, or I've missed something maybe and I'm wrong ?

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 6, 2018

Fix the errors by placing std:: in front of the offending function/type definition. Be careful that you're fixing the actual compile error and not changing from a [boost or fc function] to a [standard library function].

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 6, 2018

Author

@jmjatlanta there are placed already std:: but compiler cannot see the definition and that's why I've compile errors, how is it possible to use something without including it ?

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 6, 2018

Oops, sorry. I misunderstood what abitmore was asking. Please ignore my above comment. Post your compile errors somewhere (I recommend http://pastebin.com, as it doesn't mess up formatting), and I'll give you some pointers.

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 6, 2018

Author

@jmjatlanta no worries :-) Thanks for your efforts ! Here is the link https://pastebin.com/fd8NgAqJ

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 6, 2018

Yes, it looks like you interpreted Abitmore's comment incorrectly as did I. He's asking you to remove line 14 above. The enum.

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 6, 2018

Member

I mean remove the line enum mode { out, binary }; to prevent it from being (mis)used.

Your compiler errors seem not related to the change. What have you changed?

This comment has been minimized.

Copy link
@nanomobile

nanomobile Jul 6, 2018

Author

@jmjatlanta yes you're absolutely right that I've misunderstood @abitmore 's suggestion :-)
@abitmore I've thought that you told about next lines of code which are selected as green in your comment :

#include

using namespace std;

Now I've understood your suggestion correctly about line 14

@jmjatlanta

This comment has been minimized.

Copy link

commented Jul 6, 2018

@abitmore test case: Yes. I designed that test case to do a quick startup, throw some stuff in the log, and shut down. Requires manual verification. Since we control the log messages, it wouldn't be too hard to put some checks in there. Here's what I'm thinking...

  • create a log file with some text in it at startup, make sure the line still exists in the end (making sure the test is set up to not roll that file off)
  • make sure the timestamp of the last log message in each file is older than the first one of the next file

@nanomobile nanomobile force-pushed the nanomobile:valera_issue_809 branch from cdec042 to 6dd130c Jul 7, 2018

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jul 7, 2018

is ready

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jul 7, 2018

@jmjatlanta should I improve your test further or do you want to do it yourself ?

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 7, 2018

@nanomobile please link the code committer account "Valera Cogut" to your github account. https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 7, 2018

@jmjatlanta is on vacation, so @nanomobile please make your own decision :)

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jul 7, 2018

@abitmore ok sure ! Thank you !

@nanomobile nanomobile force-pushed the nanomobile:valera_issue_809 branch from 6dd130c to 5a9adeb Jul 7, 2018

@nanomobile nanomobile force-pushed the nanomobile:valera_issue_809 branch from 5a9adeb to 20724f7 Jul 9, 2018

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jul 9, 2018

guys, check test please now and all other details for this issue in order to close this as soon as possible :-)

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

@nanomobile just a note, in the future, please add new code with new commits but not always replace the old commit, to make it easier for others to know what you've newly changed, make the review work easier. Thanks.

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jul 9, 2018

@abitmore yes sure ! Sorry, I'm for 1st time cannot do everything correctly 100% :-) But now I know how to make all things correctly ! Thanks !

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jul 9, 2018

@abitmore did you close issue #809 ? I see it is closed now, but cannot understand because I see that it was closed by me :-) but I do not have permissions to close issues as I know, am I wrong ? and if it was closed in bitshares-core then it must be closed as well here for bitshares-fc ? right ? Help me please to understand it :-)

Thanks !

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

as issue creator you can close the issues you started with no special authorization. same with pull requests.

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jul 9, 2018

@oxarbitrage thanks for the info ! didn't know it, and I was wondered if the issue was closed by me because I didn't close it, I've just pushed my commits ... should I open it again ? or who should open it again ? who can help me with it please ? or it must be closed, I wanted to make rebase also in bitshares-core repo. Here in bitshares-fc all is OK as I can see. I've been a bit confused with 2 pull requests for 2 repos bitshares-core and bitshares-fc :-)

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

@nanomobile when you removed your branch, github will close the related pull. That's why I said above: add commits.

@nanomobile

This comment has been minimized.

Copy link
Author

commented Jul 9, 2018

@abitmore thanks for the info, yes I know this, but I've deleted locally only the branch, maybe I've missed something. Today was hard day for me :-) sorry !

@cogutvalera

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

guys all is OK with this pull request now ? can we close it ?

@abitmore

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

@cogutvalera most people in core dev team are now on vacation, to be prepared for the upcoming consensus upgrade (July 19). Please be a little patient, no need to ask everyday ;-)

@cogutvalera

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

@abitmore ok sure ;-)

fc::time_point_sec start_time = fc::time_point_sec( (uint32_t)(file_number * interval_seconds) );
fc::string timestamp_string = start_time.to_non_delimited_iso_string();
fc::path link_filename = conf.filename;
fc::path log_filename = link_filename.parent_path() / (link_filename.filename().string() + "." + timestamp_string);

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 16, 2018

I believe this could be simplified if we added a member to the file_appender that is the current filename.

@abitmore what do you think? Is such a change advisable? Is it worth it? Would it be considered "scope creep"?

Advantage: Easier testing, elimination of duplicate code.
Disadvantage: It could encourage someone to open the file and add their own information in the log.
Disadvantage: This test would have to cast the fc::appender::ptr to a file_appender to get the value.
Disadvantage: By the time the test gets the filename, it could be stale due to the multithreaded nature of this appender. Although the code above suffers from the same problem.

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 16, 2018

Member

Sorry, I don't have an idea.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 17, 2018

Member

should I do this change @jmjatlanta ? guys what do you think ?

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 17, 2018

Member

Disadvantage: By the time the test gets the filename, it could be stale due to the multithreaded nature of this appender. Although the code above suffers from the same problem. - Why current test code has the same problem ? Doesn't this test code run in it's own thread to check how appender works ?

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 17, 2018

Member

so should I change this snippet of code or not ?

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 18, 2018

Member

@jmjatlanta should I change that code or not ? is this issue finished or not yet ?

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 23, 2018

I don't think changing the code is worth it, as the problem will exist regardless. There is a very slim chance that the test will fail, but I doubt anyone will run into it. I will review everything again. If the chance of failure is there, perhaps we should document it as a comment within the test.

@jmjatlanta
Copy link

left a comment

You're almost there.

@@ -3,16 +3,19 @@
#include <fc/filesystem.hpp>
#include <fc/io/iostream.hpp>

#include <fstream>

using namespace std;

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Jul 16, 2018

Placing a using clause in a header is a very bad idea. Please remove this line.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 17, 2018

Member

ok sure ! Thank you @jmjatlanta

@cogutvalera

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

guys when you will have enough time to check this PR please do it ...

@abitmore abitmore requested a review from jmjatlanta Jul 21, 2018

@nanomobile nanomobile changed the title fixed #809 issue Log file of current hour gets overwritten by default #809 Jul 21, 2018

@abitmore abitmore merged commit f1e48d3 into bitshares:master Jul 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.