spdf/elektron
Archived
1
0
Fork 0
Code Issues 4 Pull requests 1 Projects Releases Packages Wiki Activity

WIP : Elektron Logging library #16

Merged
balandi1 merged 50 commits from master into master 2019-12-10 01:15:34 +00:00
Conversation 113 Commits 50 Files changed 61 +1510 -1067
balandi1 commented 2019-11-13 21:23:46 +00:00 (Migrated from github.com)
Copy link
No description provided.
pradykaushik commented 2019-11-14 02:30:43 +00:00 (Migrated from github.com)
Copy link

@ridv this PR corresponds to retrofitting code to use logrus for logging and get rid of the built-in logging library.

@ridv this PR corresponds to retrofitting code to use logrus for logging and get rid of the built-in logging library.
pradykaushik commented 2019-11-14 02:38:25 +00:00 (Migrated from github.com)
Copy link

@balandi1 please also make a PR to elektron-vendor with the updated dependencies to make sure that dependencies are kept up to date.

@balandi1 please also make a PR to [elektron-vendor](https://github.com/spdfg/elektron-vendor) with the updated dependencies to make sure that dependencies are kept up to date.
ridv (Migrated from github.com) requested changes 2019-11-19 03:52:38 +00:00
ridv (Migrated from github.com) left a comment

Run code all modified code through goimports

Run code all modified code through goimports
def/taskUtils.go
@ -18,3 +17,4 @@
//
package def
ridv (Migrated from github.com) commented 2019-11-19 02:54:35 +00:00

Format error, fix spacing or run through goimports

Format error, fix spacing or run through [goimports](https://godoc.org/golang.org/x/tools/cmd/goimports)
ridv (Migrated from github.com) commented 2019-11-19 02:55:10 +00:00

Let's pick a better alias, this doesn't really say much about what this package holds.

Let's pick a better alias, this doesn't really say much about what this package holds.
def/taskUtils.go
@ -105,7 +107,7 @@ func clusterSizeAvgMMMPU(tasks []Task, taskObservation func(task Task) []float64
} else {
ridv (Migrated from github.com) commented 2019-11-19 02:56:12 +00:00

Is Logf an option here instead of sprinting?

Is Logf an option here instead of sprinting?
elektronLogging/ElektronFormatter.go
ridv (Migrated from github.com) commented 2019-11-19 02:56:40 +00:00

Formatting

Formatting
ridv (Migrated from github.com) commented 2019-11-19 02:57:02 +00:00

Formatting

Formatting
ridv (Migrated from github.com) commented 2019-11-19 02:59:56 +00:00

Use strings.join instead. You should create the slice and append the message if necessary.

Use [strings.join](https://golang.org/pkg/strings/#Join) instead. You should create the slice and append the message if necessary.
ridv (Migrated from github.com) commented 2019-11-19 03:00:54 +00:00

This seems really inefficient. Wouldn't value already have to be a string for this to succeed? In which case, I don't think we gain anything here by suing a sprintf.

This seems really inefficient. Wouldn't value already have to be a string for this to succeed? In which case, I don't think we gain anything here by suing a sprintf.
elektronLogging/clsfnTaskDistOverheadLogger.go
ridv (Migrated from github.com) commented 2019-11-19 03:01:56 +00:00

Use struct literals instead of new as it's more descriptive:
&ClsfnTaskDistOverheadLogger{}

Use struct literals instead of new as it's more descriptive: `&ClsfnTaskDistOverheadLogger{}`
ridv (Migrated from github.com) commented 2019-11-19 03:49:35 +00:00

Where is log dir coming from? Shouldn't be a global

Where is log dir coming from? Shouldn't be a global
elektronLogging/createLogDir.go
ridv (Migrated from github.com) commented 2019-11-19 03:50:51 +00:00

Why not use string.join([]string{...}, "") ?

Why not use `string.join([]string{...}, "")` ?
elektronLogging/loggerChain.go
ridv (Migrated from github.com) commented 2019-11-19 03:52:10 +00:00

If the code isn't needed it shouldn't be left here. If the commented out code is still needed there should be some justification for it still being here.

If the code isn't needed it shouldn't be left here. If the commented out code is still needed there should be some justification for it still being here.
balandi1 (Migrated from github.com) reviewed 2019-11-20 18:44:44 +00:00
def/taskUtils.go
@ -18,3 +17,4 @@
//
package def
balandi1 (Migrated from github.com) commented 2019-11-20 18:44:44 +00:00

Yes, I will fix all the formatting errors

Yes, I will fix all the formatting errors
balandi1 (Migrated from github.com) reviewed 2019-11-20 18:47:34 +00:00
def/taskUtils.go
@ -18,3 +17,4 @@
//
package def
balandi1 (Migrated from github.com) commented 2019-11-20 18:47:33 +00:00

Okay. Will changing it to elekLogTypes or logTypes simply work ?

Okay. Will changing it to `elekLogTypes` or `logTypes` simply work ?
balandi1 (Migrated from github.com) reviewed 2019-11-20 18:48:25 +00:00
elektronLogging/ElektronFormatter.go
balandi1 (Migrated from github.com) commented 2019-11-20 18:48:25 +00:00

Okay. Sure

Okay. Sure
balandi1 (Migrated from github.com) reviewed 2019-11-20 18:49:00 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
balandi1 (Migrated from github.com) commented 2019-11-20 18:49:00 +00:00

Okay, will do the changes

Okay, will do the changes
balandi1 (Migrated from github.com) reviewed 2019-11-20 18:52:01 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
balandi1 (Migrated from github.com) commented 2019-11-20 18:52:01 +00:00

It is defined in createLogDir.go. Since all these files belong to same package, I accessed it in this way.

It is defined in `createLogDir.go`. Since all these files belong to same package, I accessed it in this way.
balandi1 (Migrated from github.com) reviewed 2019-11-20 18:52:16 +00:00
elektronLogging/createLogDir.go
balandi1 (Migrated from github.com) commented 2019-11-20 18:52:16 +00:00

Yeah, thats right

Yeah, thats right
balandi1 (Migrated from github.com) reviewed 2019-11-20 18:52:44 +00:00
elektronLogging/loggerChain.go
balandi1 (Migrated from github.com) commented 2019-11-20 18:52:44 +00:00

Okay. Will remove the unnecessary code

Okay. Will remove the unnecessary code
balandi1 (Migrated from github.com) reviewed 2019-11-20 19:42:42 +00:00
elektronLogging/ElektronFormatter.go
balandi1 (Migrated from github.com) commented 2019-11-20 19:42:41 +00:00

value is not a string here, it is of type interface{}. But to avoid using sprintf, would it be fine to use type assertion? For eg : value.(string)

value is not a string here, it is of type interface{}. But to avoid using sprintf, would it be fine to use type assertion? For eg : `value.(string) `
pradykaushik commented 2019-11-21 00:30:04 +00:00 (Migrated from github.com)
Copy link

Just checked the code and made a test run.

  1. Seems like the log prefix provided using the -logPrefix commandline option is not used when creating the log directory.
  2. Color codes should be removed from the ESC[32;1m[INFO]:ESC[0m prefix on each line in the logs.
  3. PCP log file has ESC[32;1m[INFO]:ESC[0m prefix on each line. PCP logs should not have this prefix and should just have the timestamp and the message. One way of making this generic is by supporting overriding of log config for each element in the chain. For example, PCP logs are to be persisted in a different format than the universal format specified for all others.
  4. If a log type is disabled in (specifying enabled: false in the logConfig yaml file), then the corresponding log file should not be created. This would result in no log file being empty.
Just checked the code and made a test run. 1. Seems like the log prefix provided using the `-logPrefix` commandline option is not used when creating the log directory. 2. Color codes should be removed from the `ESC[32;1m[INFO]:ESC[0m ` prefix on each line in the logs. 2. PCP log file has `ESC[32;1m[INFO]:ESC[0m ` prefix on each line. PCP logs should not have this prefix and should just have the timestamp and the message. One way of making this generic is by supporting overriding of log config for each element in the chain. For example, PCP logs are to be persisted in a different format than the universal format specified for all others. 3. If a log type is disabled in (specifying `enabled: false` in the logConfig yaml file), then the corresponding log file should not be created. This would result in no log file being empty.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 00:31:57 +00:00
def/taskUtils.go
pradykaushik (Migrated from github.com) commented 2019-11-21 00:31:57 +00:00

I would alias this to elekLog as you seem to be using that prefix when aliasing imports of sub-packages.

I would alias this to _elekLog_ as you seem to be using that prefix when aliasing imports of sub-packages.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 00:36:34 +00:00
def/taskUtils.go
pradykaushik (Migrated from github.com) commented 2019-11-21 00:36:33 +00:00

This is very confusing. In fact, now that we are using logrus, we can get rid of the following types in types.go.

  1. ERROR
  2. GENERAL
  3. SUCCESS
  4. WARNING

Instead, we should just mention that we are logging to the console. The log level is anyway specified as the second argument.

This is very confusing. In fact, now that we are using logrus, we can get rid of the following types in `types.go`. 1. ERROR 2. GENERAL 3. SUCCESS 4. WARNING Instead, we should just mention that we are logging to the console. The log level is anyway specified as the second argument.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 00:38:19 +00:00
def/taskUtils.go
@ -105,7 +107,7 @@ func clusterSizeAvgMMMPU(tasks []Task, taskObservation func(task Task) []float64
} else {
pradykaushik (Migrated from github.com) commented 2019-11-21 00:38:18 +00:00

Replace elekLogT.ERROR with elekLogT.CONSOLE.

Replace `elekLogT.ERROR` with `elekLogT.CONSOLE`.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 00:39:20 +00:00
def/taskUtils.go
@ -105,7 +107,7 @@ func clusterSizeAvgMMMPU(tasks []Task, taskObservation func(task Task) []float64
} else {
pradykaushik (Migrated from github.com) commented 2019-11-21 00:39:20 +00:00

err.Error() returns the string representation.

`err.Error()` returns the string representation.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 00:50:07 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 00:50:06 +00:00

I think it will be better if it is encapsulated.
Something like

type logDir struct {
    name string
    // other fields if necessary.
}
I think it will be better if it is encapsulated. Something like ```go type logDir struct { name string // other fields if necessary. } ```
pradykaushik (Migrated from github.com) reviewed 2019-11-21 00:52:08 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 00:52:08 +00:00

Maybe refactor to ClsfnTaskDistrOverheadLogger so that it is clear that we are intending "Distribution" and now "Distance"?

Maybe refactor to `ClsfnTaskDistrOverheadLogger` so that it is clear that we are intending "Distribution" and now "Distance"?
pradykaushik (Migrated from github.com) reviewed 2019-11-21 00:57:11 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 00:57:11 +00:00

You should use filepath.Join(...) to keep it generic.

You should use [filepath.Join(...)](https://golang.org/src/path/filepath/path.go?s=5893:5925#L199) to keep it generic.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 01:06:38 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 01:06:38 +00:00

You can possible rewrite this function to something like the one shown below.

func (cLog *ClsfnTaskDistOverheadLogger) SetLogFile(prefix string) {
    filename := prefix + config.TaskDistConfig.FilenameExtension
    if logDir != "" {
        if logFile, err := os.Create(filepath.Join(logDir, filename)); err != nil {
            log.Fatal("Unable to create logFile: ", err)
        } else {
            cLog.LogFileName = logFile
            cLog.AllowOnConsole = config.TaskDistConfig.AllowOnConsole
        }
    }
}
You can possible rewrite this function to something like the one shown below. ```go func (cLog *ClsfnTaskDistOverheadLogger) SetLogFile(prefix string) { filename := prefix + config.TaskDistConfig.FilenameExtension if logDir != "" { if logFile, err := os.Create(filepath.Join(logDir, filename)); err != nil { log.Fatal("Unable to create logFile: ", err) } else { cLog.LogFileName = logFile cLog.AllowOnConsole = config.TaskDistConfig.AllowOnConsole } } } ```
pradykaushik (Migrated from github.com) reviewed 2019-11-21 01:10:18 +00:00
elektronLogging/createLogDir.go
pradykaushik (Migrated from github.com) commented 2019-11-21 01:10:18 +00:00

Extend the comment to show how the format of the log directory name is going to look.

Extend the comment to show how the format of the log directory name is going to look.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 01:18:14 +00:00
elektronLogging/consoleLogger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 01:18:14 +00:00

I see why you are directly assigning true. However, to allow disabling console logs to stdout retrofit this to cLog.AllowOnConsole = config.ConsoleConfig.AllowOnConsole.

I see why you are directly assigning `true`. However, to allow disabling console logs to stdout retrofit this to `cLog.AllowOnConsole = config.ConsoleConfig.AllowOnConsole`.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 01:19:20 +00:00
elektronLogging/createLogDir.go
pradykaushik (Migrated from github.com) commented 2019-11-21 01:19:20 +00:00

Keep the alias for logrus import consistent. So, refactor this to "log".

Keep the alias for logrus import consistent. So, refactor this to "log".
pradykaushik (Migrated from github.com) reviewed 2019-11-21 01:22:15 +00:00
elektronLogging/logger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 01:22:14 +00:00

Logger and Log are synonyms and therefore should not be used interchangeably.

Logger and Log are synonyms and therefore should not be used interchangeably.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 01:23:29 +00:00
elektronLogging/logger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 01:23:28 +00:00

Comments should start with capital letters and should end with a full stop.

Comments should start with capital letters and should end with a full stop.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 01:26:48 +00:00
elektronLogging/logger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 01:26:47 +00:00

The second argument to GetLogDir() is a prefix. How is "_" a prefix? This is probably why the log directory name is not ending up prefixed with the one specified with the -logPrefix flag.

The second argument to GetLogDir() is a prefix. How is "_" a prefix? This is probably why the log directory name is not ending up prefixed with the one specified with the `-logPrefix` flag.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 01:31:25 +00:00
elektronLogging/consoleLogger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 01:31:25 +00:00

Why the pointer receiver? Technically, this function should not lead to change of state.

Why the pointer receiver? Technically, this function should not lead to change of state.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 01:33:09 +00:00
elektronLogging/logger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 01:33:09 +00:00

What is the rationale behind having a dummy node as the head of the chain?

What is the rationale behind having a dummy node as the head of the chain?
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:36:05 +00:00
elektronLogging/loggerChain.go
pradykaushik (Migrated from github.com) commented 2019-11-21 02:36:05 +00:00

As fields are optional, it is better to either have them as the last argument or not have them in the argument list at all.
I would in fact also maintain an internal data structure to store Fields that is cleared only after the log operation passes through the entire chain. You can then add a WithFields(...) method that wraps around logrus.WithFields(...).
I have created this sample code for reference. The sample code also shows how formatted strings are corresponding args can be passed and we can wrap around logrus.Logf(...) for that.

As fields are optional, it is better to either have them as the last argument or not have them in the argument list at all. I would in fact also maintain an internal data structure to store Fields that is cleared only after the log operation passes through the entire chain. You can then add a `WithFields(...)` method that wraps around `logrus.WithFields(...)`. I have created [this sample code](https://play.golang.org/p/RILTxcZtT2w) for reference. The sample code also shows how formatted strings are corresponding args can be passed and we can wrap around `logrus.Logf(...)` for that.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:43:54 +00:00
elektronLogging/loggerConfig.go
pradykaushik (Migrated from github.com) commented 2019-11-21 02:43:54 +00:00

Are column headers being added if enabled?

Are column headers being added if enabled?
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:45:06 +00:00
elektronLogging/loggerConfig.go
pradykaushik (Migrated from github.com) commented 2019-11-21 02:45:05 +00:00

Refactor to TaskDistrConfig for semantics.

Refactor to TaskDistrConfig for semantics.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:45:19 +00:00
elektronLogging/loggerConfig.go
pradykaushik (Migrated from github.com) commented 2019-11-21 02:45:19 +00:00

ditto.

ditto.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:47:43 +00:00
elektronLogging/loggerConfig.go
pradykaushik (Migrated from github.com) commented 2019-11-21 02:47:43 +00:00

The error message does not do justice in indicating the reason for the error.

The error message does not do justice in indicating the reason for the error.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:47:54 +00:00
elektronLogging/loggerConfig.go
pradykaushik (Migrated from github.com) commented 2019-11-21 02:47:53 +00:00

ditto.

ditto.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:51:18 +00:00
elektronLogging/loggerChain.go
pradykaushik (Migrated from github.com) commented 2019-11-21 02:51:18 +00:00

Refactor to LogFile as it is a pointer to os.File and not string.

Refactor to `LogFile` as it is a pointer to os.File and not string.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:51:27 +00:00
elektronLogging/pcpLogger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 02:51:27 +00:00

Refactor to PCPLogger.

Refactor to `PCPLogger`.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:56:22 +00:00
elektronLogging/pcpLogger.go
pradykaushik (Migrated from github.com) commented 2019-11-21 02:56:21 +00:00

Why not just do this?

return &PCPLogger{ 
    LoggerIml{
        Type: logType
        LogFile: CreateLogFile(prefix),
    }
}

Where func CreateLogFile(prefix string) string replaces func SetLogFile(prefix string).

Why not just do this? ```go return &PCPLogger{ LoggerIml{ Type: logType LogFile: CreateLogFile(prefix), } } ``` Where `func CreateLogFile(prefix string) string` replaces `func SetLogFile(prefix string)`.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:58:17 +00:00
environment/env.go
pradykaushik (Migrated from github.com) commented 2019-11-21 02:58:17 +00:00

This file is intended to only store environment variables.

This file is intended to only store environment variables.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:59:07 +00:00
logConfig.yaml
pradykaushik (Migrated from github.com) commented 2019-11-21 02:59:07 +00:00

Inconsistent spacing.

Inconsistent spacing.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 02:59:18 +00:00
logConfig.yaml
pradykaushik (Migrated from github.com) commented 2019-11-21 02:59:18 +00:00

unnecessary comment.

unnecessary comment.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 03:01:56 +00:00
schedulers/helpers.go
@ -129,14 +131,6 @@ func WithPCPLog(pcpLog chan struct{}) SchedulerOptions {
}
}
pradykaushik (Migrated from github.com) commented 2019-11-21 03:01:56 +00:00

Remove commented code if not required.

Remove commented code if not required.
balandi1 (Migrated from github.com) reviewed 2019-11-21 18:07:17 +00:00
def/taskUtils.go
@ -105,7 +107,7 @@ func clusterSizeAvgMMMPU(tasks []Task, taskObservation func(task Task) []float64
} else {
balandi1 (Migrated from github.com) commented 2019-11-21 18:07:17 +00:00

Yes. I will work on implementing Logf()

Yes. I will work on implementing Logf()
balandi1 (Migrated from github.com) reviewed 2019-11-21 18:07:31 +00:00
elektronLogging/ElektronFormatter.go
balandi1 (Migrated from github.com) commented 2019-11-21 18:07:31 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-21 18:07:42 +00:00
elektronLogging/ElektronFormatter.go
balandi1 (Migrated from github.com) commented 2019-11-21 18:07:42 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-21 18:48:27 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
balandi1 (Migrated from github.com) commented 2019-11-21 18:48:26 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-21 19:30:07 +00:00
def/taskUtils.go
balandi1 (Migrated from github.com) commented 2019-11-21 19:30:07 +00:00

Okay. Done

Okay. Done
balandi1 (Migrated from github.com) reviewed 2019-11-21 19:59:46 +00:00
def/taskUtils.go
balandi1 (Migrated from github.com) commented 2019-11-21 19:59:46 +00:00

Yeah, that's right.

Yeah, that's right.
balandi1 (Migrated from github.com) reviewed 2019-11-21 19:59:56 +00:00
def/taskUtils.go
@ -105,7 +107,7 @@ func clusterSizeAvgMMMPU(tasks []Task, taskObservation func(task Task) []float64
} else {
balandi1 (Migrated from github.com) commented 2019-11-21 19:59:56 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-21 20:07:54 +00:00
def/taskUtils.go
@ -105,7 +107,7 @@ func clusterSizeAvgMMMPU(tasks []Task, taskObservation func(task Task) []float64
} else {
balandi1 (Migrated from github.com) commented 2019-11-21 20:07:53 +00:00

Okay. Will do the change

Okay. Will do the change
balandi1 (Migrated from github.com) reviewed 2019-11-21 20:11:44 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
balandi1 (Migrated from github.com) commented 2019-11-21 20:11:44 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-21 20:27:31 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
balandi1 (Migrated from github.com) commented 2019-11-21 20:27:31 +00:00

Okay. Sure

Okay. Sure
pradykaushik (Migrated from github.com) reviewed 2019-11-21 22:13:22 +00:00
elektronLogging/ElektronFormatter.go
pradykaushik (Migrated from github.com) commented 2019-11-21 22:13:22 +00:00

Line 43 and 44 can fit in one line. We currently do not have any style linting setup.

Line 43 and 44 can fit in one line. We currently do not have any style linting setup.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 22:19:08 +00:00
schedulers/base.go
pradykaushik (Migrated from github.com) commented 2019-11-21 22:19:07 +00:00

refactor variable name to logMsgType. In fact, you don't need to have this variable at all. Instead, you can just use elekLogTypes.CONSOLE directly.
Make the changes in subsequent occurrences too.

refactor variable name to `logMsgType`. In fact, you don't need to have this variable at all. Instead, you can just use `elekLogTypes.CONSOLE` directly. Make the changes in subsequent occurrences too.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 22:29:05 +00:00
schedulers/base.go
pradykaushik (Migrated from github.com) commented 2019-11-21 22:29:05 +00:00

type mismatch. You might want to use either %v or you can use masterInfo.GetHostname().

type mismatch. You might want to use either %v or you can use [masterInfo.GetHostname()](https://github.com/mesos/mesos-go/blob/29de6ff97b48c29cb5ac07029ed75186e5ba0eed/api/v0/mesosproto/mesos.pb.go#L1581).
pradykaushik (Migrated from github.com) reviewed 2019-11-21 22:29:36 +00:00
schedulers/base.go
pradykaushik (Migrated from github.com) commented 2019-11-21 22:29:36 +00:00

Will fit on a single line.

Will fit on a single line.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 22:32:31 +00:00
schedulers/helpers.go
@ -20,24 +20,26 @@ package schedulers
import (
"fmt"
pradykaushik (Migrated from github.com) commented 2019-11-21 22:32:31 +00:00

Here, task would be the index.

    for _, task := range tasks {
        // rest of the code.
    }
Here, `task` would be the index. ```go for _, task := range tasks { // rest of the code. } ```
pradykaushik (Migrated from github.com) reviewed 2019-11-21 22:34:08 +00:00
utilities/offerUtils/offerUtils.go
pradykaushik (Migrated from github.com) commented 2019-11-21 22:34:07 +00:00

As host is a string, you don't need to use fmt.Sprintf.

As `host` is a string, you don't need to use `fmt.Sprintf`.
pradykaushik (Migrated from github.com) reviewed 2019-11-21 22:34:32 +00:00
utilities/offerUtils/offerUtils.go
pradykaushik (Migrated from github.com) commented 2019-11-21 22:34:31 +00:00

ditto for host and class.

ditto for `host` and `class`.
balandi1 (Migrated from github.com) reviewed 2019-11-21 22:47:38 +00:00
elektronLogging/consoleLogger.go
balandi1 (Migrated from github.com) commented 2019-11-21 22:47:38 +00:00

Sure

Sure
balandi1 (Migrated from github.com) reviewed 2019-11-21 22:47:53 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
balandi1 (Migrated from github.com) commented 2019-11-21 22:47:52 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-21 22:52:32 +00:00
elektronLogging/consoleLogger.go
balandi1 (Migrated from github.com) commented 2019-11-21 22:52:32 +00:00

Yeah, thats right. I will change it

Yeah, thats right. I will change it
pradykaushik (Migrated from github.com) reviewed 2019-11-21 22:59:52 +00:00
elektronLogging/ElektronFormatter.go
pradykaushik (Migrated from github.com) commented 2019-11-21 22:59:52 +00:00

Peeked at how logrus serializes values and seems like fmt.Sprint is used if type assertion fails.

Peeked at how [logrus](https://github.com/sirupsen/logrus/blob/67a7fdcf741f4d5cee82cb9800994ccfd4393ad0/text_formatter.go#L315) serializes values and seems like fmt.Sprint is used if type assertion fails.
ridv (Migrated from github.com) reviewed 2019-11-24 18:43:56 +00:00
def/taskUtils.go
@ -18,3 +17,4 @@
//
package def
ridv (Migrated from github.com) commented 2019-11-24 18:43:55 +00:00

logTypes would be fine

logTypes would be fine
ridv (Migrated from github.com) reviewed 2019-11-24 18:46:09 +00:00
elektronLogging/ElektronFormatter.go
ridv (Migrated from github.com) commented 2019-11-24 18:46:09 +00:00

ahh alright, I thought it was a string coming in. Alright, if it's been sanity checked then i'm good with it.

ahh alright, I thought it was a string coming in. Alright, if it's been sanity checked then i'm good with it.
ridv (Migrated from github.com) reviewed 2019-11-24 18:46:34 +00:00
elektronLogging/clsfnTaskDistOverheadLogger.go
ridv (Migrated from github.com) commented 2019-11-24 18:46:34 +00:00

looks good now thanks!

looks good now thanks!
ridv commented 2019-11-24 18:50:41 +00:00 (Migrated from github.com)
Copy link

Thanks @balandi1 .This was a big undertaking and I really appreciate the effort you're putting towards getting it merged. Looks good to me as soon as all of @pradykaushik 's concerns are addressed. Very excited to have you be part of the list of contributors once this is merged.

Thanks @balandi1 .This was a big undertaking and I really appreciate the effort you're putting towards getting it merged. Looks good to me as soon as all of @pradykaushik 's concerns are addressed. Very excited to have you be part of the list of contributors once this is merged.
balandi1 (Migrated from github.com) reviewed 2019-11-26 17:08:07 +00:00
elektronLogging/loggerConfig.go
balandi1 (Migrated from github.com) commented 2019-11-26 17:08:06 +00:00

That was for future purpose. But I have removed it now

That was for future purpose. But I have removed it now
balandi1 (Migrated from github.com) reviewed 2019-11-26 17:44:22 +00:00
elektronLogging/logger.go
balandi1 (Migrated from github.com) commented 2019-11-26 17:44:22 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 17:44:44 +00:00
elektronLogging/createLogDir.go
balandi1 (Migrated from github.com) commented 2019-11-26 17:44:43 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 17:45:43 +00:00
elektronLogging/loggerConfig.go
balandi1 (Migrated from github.com) commented 2019-11-26 17:45:42 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 17:45:56 +00:00
elektronLogging/loggerConfig.go
balandi1 (Migrated from github.com) commented 2019-11-26 17:45:56 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 17:46:29 +00:00
elektronLogging/loggerConfig.go
balandi1 (Migrated from github.com) commented 2019-11-26 17:46:29 +00:00

Changed the error message

Changed the error message
balandi1 (Migrated from github.com) reviewed 2019-11-26 17:46:42 +00:00
elektronLogging/loggerConfig.go
balandi1 (Migrated from github.com) commented 2019-11-26 17:46:41 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 17:46:53 +00:00
elektronLogging/loggerChain.go
balandi1 (Migrated from github.com) commented 2019-11-26 17:46:53 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 17:47:04 +00:00
elektronLogging/pcpLogger.go
balandi1 (Migrated from github.com) commented 2019-11-26 17:47:03 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:06:20 +00:00
environment/env.go
balandi1 (Migrated from github.com) commented 2019-11-26 18:06:19 +00:00

Removed it from here. Added a flag in scheduler.go

Removed it from here. Added a flag in scheduler.go
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:11:54 +00:00
logConfig.yaml
balandi1 (Migrated from github.com) commented 2019-11-26 18:11:53 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:12:07 +00:00
logConfig.yaml
balandi1 (Migrated from github.com) commented 2019-11-26 18:12:07 +00:00

Removed

Removed
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:12:58 +00:00
schedulers/helpers.go
@ -129,14 +131,6 @@ func WithPCPLog(pcpLog chan struct{}) SchedulerOptions {
}
}
balandi1 (Migrated from github.com) commented 2019-11-26 18:12:58 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:14:35 +00:00
elektronLogging/ElektronFormatter.go
balandi1 (Migrated from github.com) commented 2019-11-26 18:14:34 +00:00

Okay

Okay
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:26:21 +00:00
schedulers/base.go
balandi1 (Migrated from github.com) commented 2019-11-26 18:26:20 +00:00

Okay. Done

Okay. Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:28:46 +00:00
schedulers/base.go
balandi1 (Migrated from github.com) commented 2019-11-26 18:28:45 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:32:57 +00:00
schedulers/base.go
balandi1 (Migrated from github.com) commented 2019-11-26 18:32:57 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:34:17 +00:00
schedulers/helpers.go
@ -20,24 +20,26 @@ package schedulers
import (
"fmt"
balandi1 (Migrated from github.com) commented 2019-11-26 18:34:17 +00:00

Okay

Okay
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:35:46 +00:00
utilities/offerUtils/offerUtils.go
balandi1 (Migrated from github.com) commented 2019-11-26 18:35:45 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:36:17 +00:00
utilities/offerUtils/offerUtils.go
balandi1 (Migrated from github.com) commented 2019-11-26 18:36:16 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:53:16 +00:00
elektronLogging/logger.go
balandi1 (Migrated from github.com) commented 2019-11-26 18:53:15 +00:00

Nothing specific. I had kept ElektronLog as LoggerImpl type. But I have changed to ConsoleLogger rather. Works fine now

Nothing specific. I had kept `ElektronLog` as `LoggerImpl` type. But I have changed to `ConsoleLogger` rather. Works fine now
balandi1 (Migrated from github.com) reviewed 2019-11-26 18:58:23 +00:00
elektronLogging/logger.go
balandi1 (Migrated from github.com) commented 2019-11-26 18:58:23 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-11-26 19:05:09 +00:00
elektronLogging/logger.go
balandi1 (Migrated from github.com) commented 2019-11-26 19:05:09 +00:00

Changed ElektronLog to ElektronLogger

Changed `ElektronLog` to `ElektronLogger`
balandi1 (Migrated from github.com) reviewed 2019-11-26 19:07:02 +00:00
elektronLogging/createLogDir.go
balandi1 (Migrated from github.com) commented 2019-11-26 19:07:01 +00:00

Done

Done
ridv (Migrated from github.com) approved these changes 2019-12-02 23:23:56 +00:00
balandi1 (Migrated from github.com) reviewed 2019-12-04 18:18:21 +00:00
elektronLogging/loggerChain.go
balandi1 (Migrated from github.com) commented 2019-12-04 18:18:20 +00:00

Done

Done
pradykaushik (Migrated from github.com) reviewed 2019-12-06 22:00:44 +00:00
logConfig.yaml
pradykaushik (Migrated from github.com) commented 2019-12-06 22:00:44 +00:00

disable schedWindow logging in the log config file that is kept within the repository.

disable schedWindow logging in the log config file that is kept within the repository.
pradykaushik (Migrated from github.com) reviewed 2019-12-06 22:00:51 +00:00
logConfig.yaml
pradykaushik (Migrated from github.com) commented 2019-12-06 22:00:51 +00:00

ditto

ditto
pradykaushik (Migrated from github.com) reviewed 2019-12-06 22:03:00 +00:00
logging/elektronFormatter.go
@ -0,0 +30,4 @@
TimestampFormat string
}
func (f elektronFormatter) Format(entry *log.Entry) ([]byte, error) {
pradykaushik (Migrated from github.com) commented 2019-12-06 22:03:00 +00:00

Does the formatter adhere to the format specified in the config provided?

Does the formatter adhere to the format specified in the config provided?
pradykaushik (Migrated from github.com) reviewed 2019-12-06 22:03:57 +00:00
logging/logger.go
pradykaushik (Migrated from github.com) commented 2019-12-06 22:03:56 +00:00

isEnabled() does not have to be part of the interface.

`isEnabled()` does not have to be part of the interface.
pradykaushik (Migrated from github.com) reviewed 2019-12-06 22:04:03 +00:00
logging/logger.go
pradykaushik (Migrated from github.com) commented 2019-12-06 22:04:02 +00:00

ditto

ditto
pradykaushik (Migrated from github.com) reviewed 2019-12-06 22:04:11 +00:00
logging/logger.go
pradykaushik (Migrated from github.com) commented 2019-12-06 22:04:11 +00:00

ditto

ditto
pradykaushik (Migrated from github.com) reviewed 2019-12-06 22:05:08 +00:00
logging/loggerConfig.go
pradykaushik (Migrated from github.com) commented 2019-12-06 22:05:07 +00:00

does this need to be exported outside the package?

does this need to be exported outside the package?
pradykaushik (Migrated from github.com) reviewed 2019-12-06 22:08:08 +00:00
scheduler.go
pradykaushik (Migrated from github.com) commented 2019-12-06 22:08:07 +00:00

change the shorthand to logCfg

change the shorthand to `logCfg`
pradykaushik (Migrated from github.com) reviewed 2019-12-06 22:08:51 +00:00
scheduler.go
pradykaushik (Migrated from github.com) commented 2019-12-06 22:08:51 +00:00

In the future, we can have the log prefix to be part of the log config.

In the future, we can have the log prefix to be part of the log config.
pradykaushik (Migrated from github.com) reviewed 2019-12-06 22:10:06 +00:00
utilities/schedUtils/schedUtils.go
pradykaushik (Migrated from github.com) commented 2019-12-06 22:10:06 +00:00

can fit in two lines.

can fit in two lines.
pradykaushik commented 2019-12-06 22:12:36 +00:00 (Migrated from github.com)
Copy link

Overall, LGTM.
I have mentioned a few nit picks plus a formatting related fix.
Once those are fixed, I think this is ready to be merged in.
Thank you for undertaking a non-trivial task. This was an important addition to elektron.

Overall, LGTM. I have mentioned a few nit picks plus a formatting related fix. Once those are fixed, I think this is ready to be merged in. Thank you for undertaking a non-trivial task. This was an important addition to elektron.
balandi1 (Migrated from github.com) reviewed 2019-12-09 15:37:12 +00:00
logConfig.yaml
balandi1 (Migrated from github.com) commented 2019-12-09 15:37:11 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-12-09 15:37:36 +00:00
logConfig.yaml
balandi1 (Migrated from github.com) commented 2019-12-09 15:37:36 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-12-09 15:40:05 +00:00
logging/elektronFormatter.go
@ -0,0 +30,4 @@
TimestampFormat string
}
func (f elektronFormatter) Format(entry *log.Entry) ([]byte, error) {
balandi1 (Migrated from github.com) commented 2019-12-09 15:40:04 +00:00

No, actually as per our discussion earlier you had suggested to implement a decorator pattern for using the format specified in config. So currently, it has a fixed format and there is technically no use of the format specified in config file.

No, actually as per our discussion earlier you had suggested to implement a decorator pattern for using the format specified in config. So currently, it has a fixed format and there is technically no use of the format specified in config file.
balandi1 (Migrated from github.com) reviewed 2019-12-09 15:41:29 +00:00
logging/logger.go
balandi1 (Migrated from github.com) commented 2019-12-09 15:41:29 +00:00

Yes. Removed it from there

Yes. Removed it from there
balandi1 (Migrated from github.com) reviewed 2019-12-09 15:42:14 +00:00
logging/logger.go
balandi1 (Migrated from github.com) commented 2019-12-09 15:42:13 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-12-09 15:42:23 +00:00
logging/logger.go
balandi1 (Migrated from github.com) commented 2019-12-09 15:42:23 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-12-09 15:47:23 +00:00
scheduler.go
balandi1 (Migrated from github.com) commented 2019-12-09 15:47:22 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-12-09 15:48:29 +00:00
scheduler.go
balandi1 (Migrated from github.com) commented 2019-12-09 15:48:29 +00:00

Oh, okay.

Oh, okay.
balandi1 (Migrated from github.com) reviewed 2019-12-09 15:49:43 +00:00
utilities/schedUtils/schedUtils.go
balandi1 (Migrated from github.com) commented 2019-12-09 15:49:43 +00:00

Done

Done
balandi1 (Migrated from github.com) reviewed 2019-12-09 16:03:06 +00:00
logging/loggerConfig.go
balandi1 (Migrated from github.com) commented 2019-12-09 16:03:06 +00:00

No, not needed. Changed it to getConfig .

No, not needed. Changed it to `getConfig` .
This repository is archived. You cannot comment on pull requests.
Reviewers
No reviewers
ridv
Labels
Clear labels   
bug

Something isn't working

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
fix

Fix files/design

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
major

of high importance

  
question

Further information is requested

  
testing

add test code

  
wontfix

This will not be worked on

No labels
bug
documentation
duplicate
enhancement
fix
good first issue
help wanted
invalid
major
question
testing
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: spdf/elektron#16
No description provided.
Delete branch "master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?