search

dmarro89 / dare-db

Dare-DB is a lightweight in-memory database written in Go, featuring Redis-inspired hashtables and HTTP/HTTPS endpoints for seamless data storage and retrieval, with Docker support for easy deployment
MIT License
16 stars 1 forks source link

Creating DareLogger struct, restructuring logger and configuration struct #27

Closed dmarro89 closed 2 weeks ago

dmarro89 commented 2 weeks ago

I've tried to restrucutring Logger and Configurator in order to keep them clean.

vdmitriyev commented 2 weeks ago

Thanks. Now it looks better structured. Thank you! I found only two issues (or strange behavior).

  1. After the server has been stopped, it is not able to write to logfile (file is closed). Example output:

    2024-06-14 15:21:19 [INFO] - Graceful shutdown complete.
Failed to write to log, write daredb.log: file already closed

  2. Logs from the configuration phase will not be saved in log file. But they appear in the STDOUT/console. Which is fine for now, but this behavior could be corrected/change later. Here is an example output from console:

    2024-06-14 15:49:18 [INFO] - No configuration file was supplied. Using default value: config.toml
2024-06-14 15:49:18 [INFO] - Using configuration file: config.toml
2024-06-14 15:49:18 [INFO] - Re-reading configurations from environmental variables
dmarro89 commented 2 weeks ago

Thanks. Now it looks better structured. Thank you! I found only two issues (or strange behavior).

  1. After the server has been stopped, it is not able to write to logfile (file is closed). Example output:

2024-06-14 15:21:19 [INFO] - Graceful shutdown complete.

Failed to write to log, write daredb.log: file already closed
  1. Logs from the configuration phase will not be saved in log file. But they appear in the STDOUT/console. Which is fine for now, but this behavior could be corrected/change later. Here is an example output from console:

2024-06-14 15:49:18 [INFO] - No configuration file was supplied. Using default value: config.toml

2024-06-14 15:49:18 [INFO] - Using configuration file: config.toml

2024-06-14 15:49:18 [INFO] - Re-reading configurations from environmental variables

Ok let me get into it.

go-while commented 2 weeks ago

I've tried to restrucutring Logger and Configurator in order to keep them clean. tried? at least my DareLogger idea was good enough to try to rebuild? xD i don't care.

dmarro89 commented 2 weeks ago

I've tried to restrucutring Logger and Configurator in order to keep them clean. tried? at least my DareLogger idea was good enough to try to rebuild? xD i don't care.

HI @go-while, honestly I didn't have the opportunity to see your rebuild. Could you please open a PR on the main branch with your idea ? In this way I can take a look to it and understand better what you've done. At the moment, it's really hard to understand what you've done.

dmarro89 commented 2 weeks ago

Thanks. Now it looks better structured. Thank you! I found only two issues (or strange behavior).

  1. After the server has been stopped, it is not able to write to logfile (file is closed). Example output:
2024-06-14 15:21:19 [INFO] - Graceful shutdown complete.
Failed to write to log, write daredb.log: file already closed
  1. Logs from the configuration phase will not be saved in log file. But they appear in the STDOUT/console. Which is fine for now, but this behavior could be corrected/change later. Here is an example output from console:
2024-06-14 15:49:18 [INFO] - No configuration file was supplied. Using default value: config.toml
2024-06-14 15:49:18 [INFO] - Using configuration file: config.toml
2024-06-14 15:49:18 [INFO] - Re-reading configurations from environmental variables

I've checked the two errors.

  1. logger.Close() was called in both start and stop method of the server. I've removed it from the start server.
  2. in the NewConfiguration() method, after that the configuration has been read from the file (with the default values), the method calls the reReadConfigsFromEnv and the initDBDirectories. This was the previous logic. Maybe I'm not getting the flow but I think that these two lines can be removed. The initDBDirectories is related to the Database Data or to the configuration ? If it's not related to the configuration, I'd like to remove from it. While the reRead is useless if you've already read the config from the file. What do you think @vdmitriyev ?
vdmitriyev commented 2 weeks ago

start of the comment: https://github.com/dmarro89/dare-db/pull/27#issuecomment-2166083017 I've checked the two errors.

  1. logger.Close() was called in both start and stop method of the server. I've removed it from the start server.
  2. in the NewConfiguration() method, after that the configuration has been read from the file (with the default values), the method calls the reReadConfigsFromEnv and the initDBDirectories. This was the previous logic. Maybe I'm not getting the flow but I think that these two lines can be removed. The initDBDirectories is related to the Database Data or to the configuration ? If it's not related to the configuration, I'd like to remove from it. While the reRead is useless if you've already read the config from the file. What do you think @vdmitriyev ?
  1. Thank you. Are you going to push the fix?
  2. A multiple points here to adress
    • I had updated the discussion thread to clarify how the configuration should work. I hope now it should be clear, why reReadConfigsFromEnv should be called - https://github.com/dmarro89/dare-db/discussions/8
    • Regarding initDBDirectories you are right - there is no need to this function call right know, when you are using HTTP server. However, in case of HTTPS server the folder settings is used to store TSL certificates. The same folder should be later on used for other purposes as well (e.g., store the roles and users in an encrypted file, etc.). The folder data is not used for now, but I hope at one point it is going to be used (e.g., to dump in-memory data as an encrypted file, when a shutdown of the server is initiated, etc.).
dmarro89 commented 2 weeks ago

start of the comment: https://github.com/dmarro89/dare-db/pull/27#issuecomment-2166083017

I've checked the two errors.

  1. logger.Close() was called in both start and stop method of the server. I've removed it from the start server.

  2. in the NewConfiguration() method, after that the configuration has been read from the file (with the default values), the method calls the reReadConfigsFromEnv and the initDBDirectories.

    This was the previous logic. Maybe I'm not getting the flow but I think that these two lines can be removed. The initDBDirectories is related to the Database Data or to the configuration ? If it's not related to the configuration, I'd like to remove from it. While the reRead is useless if you've already read the config from the file.

    What do you think @vdmitriyev ?

  1. Thank you. Are you going to push the fix?

  2. A multiple points here to adress

    • I had updated the discussion thread to clarify how the configuration should work. I hope now it should be clear, why reReadConfigsFromEnv should be called - https://github.com/dmarro89/dare-db/discussions/8

    • Regarding initDBDirectories you are right - there is no need to this function call right know, when you are using HTTP server. However, in case of HTTPS server the folder settings is used to store TSL certificates. The same folder should be later on used for other purposes as well (e.g., store the roles and users in an encrypted file, etc.). The folder data is not used for now, but I hope at one point it is going to be used (e.g., to dump in-memory data as an encrypted file, when a shutdown of the server is initiated, etc.).

Ok so, I'm going to move the db initialization in the server methods. I think that is server responsibility to create them.

vdmitriyev commented 2 weeks ago

start of the comment: https://github.com/dmarro89/dare-db/pull/27#issuecomment-2168173537

Ok so, I'm going to move the db initialization in the server methods. I think that is server responsibility to create them.

You are right that the database should be responsible for creating directories required for the start. The idea of putting that kind of initialization-code into a configuration was to have everything related to start of an application in one place (create config, read envs, create directories, etc.).

If you would like to change it, it would be also fine.

dmarro89 commented 2 weeks ago

start of the comment: #27 (comment)

Ok so, I'm going to move the db initialization in the server methods. I think that is server responsibility to create them.

You are right that the database should be responsible for creating directories required for the start. The idea of putting that kind of initialization-code into a configuration was to have everything related to start of an application in one place (create config, read envs, create directories, etc.).

If you would like to change it, it would be also fine.

I'll leave it as is. I'm going to push the fix about the Close().

vdmitriyev commented 2 weeks ago

@dmarro89

it looks like the reReadConfigsFromEnv does not work properly. One can see it during the test run of dare-db-tls docker (passed via command line envs will be ignored).

A simple test case had been added here - https://github.com/dmarro89/dare-db/pull/27/commits/4773869d71673bdd8dce39bd8b091783d5e877cf

For now, this simple test fails.

dmarro89 commented 2 weeks ago

@dmarro89

it looks like the reReadConfigsFromEnv does not work properly. One can see it during the test run of dare-db-tls docker (passed via command line envs will be ignored).

A simple test case had been added here - https://github.com/dmarro89/dare-db/pull/27/commits/4773869d71673bdd8dce39bd8b091783d5e877cf

For now, this simple test fails.

Ok, fixed. @vdmitriyev