Skip to content

Hangfire added #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Hangfire added #18

wants to merge 2 commits into from

Conversation

vmayushan
Copy link

@vmayushan vmayushan commented Sep 2, 2019

work in progress :-)

feel free to add any comments please

@@ -0,0 +1,3 @@
<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">
Copy link
Author

Choose a reason for hiding this comment

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

@mmikirtumov @lesley29
I saw it earlier in @lesley29 PR, but we don't have this dictionary file now. We were agreed to delete it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lesley29 I think that we considered to remove this file, if I am not mistaken :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we don't have this file in "scalable arch" branch https://github.com/TinkoffCreditSystems/ISA/tree/scalable_architecture/src

Copy link
Author

Choose a reason for hiding this comment

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

ok, will remove it :-)

@@ -40,10 +45,31 @@ public void ConfigureServices(IServiceCollection services)
services.AddAppDependencies();
services.AddMvc();
services.AddAutoMapper();
services.AddHangfire((serviceProvider, config) =>
Copy link
Author

Choose a reason for hiding this comment

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

Seems better to move it to Tinkoff.Isa.Infrastructure project, do you agree? to avoid duplications in API, Scheduler, SchedulerUI

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason to have Hangfire in API?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess so

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can remove it?

@@ -60,6 +86,22 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerF
}

app.UseMvc();

recurringJobManager.AddOrUpdate<JiraJob>(
Copy link
Author

Choose a reason for hiding this comment

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

I had to add this reference to have an ability to configure new providers in main ISA project.
Further this code should be in Tinkoff.ISA.Jira, Tinkoff.ISA.Confluence and so on...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, are you sure about further code placement in ISA.Jira, ISA.Confluence, etc? I supposed that we will have a core job that will ask IKnowledgeProvider for data (is it possible to create a job per each registered provider?). Is it necessary to force providers to depend on Hangfire? Correct me, if I'm wrong.

@@ -40,10 +45,31 @@ public void ConfigureServices(IServiceCollection services)
services.AddAppDependencies();
services.AddMvc();
services.AddAutoMapper();
services.AddHangfire((serviceProvider, config) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason to have Hangfire in API?

@@ -1,16 +1,21 @@
using System.IO;
using AutoMapper;
using Hangfire;
using Hangfire.Common;
using Hangfire.Mongo;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need it here?

using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Tinkoff.ISA.AppLayer;
using Tinkoff.ISA.AppLayer.Jobs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Jobs in API?

using Tinkoff.ISA.Infrastructure.Settings;
using Tinkoff.ISA.DAL;
using Tinkoff.ISA.DAL.Common;
using Tinkoff.ISA.Infrastructure.Configuration;
using Tinkoff.ISA.Infrastructure.Extensions;
using Tinkoff.ISA.Infrastructure.MongoDb;
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see any reason to add reference with MongoDb here.

@@ -40,10 +45,31 @@ public void ConfigureServices(IServiceCollection services)
services.AddAppDependencies();
services.AddMvc();
services.AddAutoMapper();
services.AddHangfire((serviceProvider, config) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can remove it?

@@ -16,7 +16,7 @@

namespace Tinkoff.ISA.AppLayer.Jobs
{
internal class ConfluenceJob : IJob
public class ConfluenceJob : IJob
Copy link
Contributor

Choose a reason for hiding this comment

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

I am going to move this to another project

"windowsAuthentication": false,
"anonymousAuthentication": true,
"iisExpress": {
"applicationUrl": "http://localhost:5000",
Copy link
Contributor

Choose a reason for hiding this comment

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

let's change the port it can bring a conflict :)

_configuration = builder.Build();
}

// This method gets called by the runtime. Use this method to add services to the container.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment :)

});
}

// This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
Copy link
Contributor

@mmikirtumov mmikirtumov Sep 6, 2019

Choose a reason for hiding this comment

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

comment remove)

{
var logSettings = _configuration.GetSection("Logging").Get<LoggingSettings>();

app.UseSerilog(loggerFactory, logSettings, "isa-.log");
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can change the logger name instead of isa-.log isa-hangfire.log?

{
CreateWebHostBuilder(args).UseUrls("http://*:5000").Build().Run();
return CreateWebHostBuilder(args).UseUrls("http://*:5000").Build().RunAsync();
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reason to change it to RunAsync? Is there any other processes?
Is there any difference in runtime level?

BackupStrategy = MongoBackupStrategy.Collections
}
});
});
}

// This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove also here :)

return Task.CompletedTask;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

new line in the end of file

@@ -1,16 +1,21 @@
using System.IO;
using AutoMapper;
using Hangfire;
using Hangfire.Common;
Copy link
Contributor

Choose a reason for hiding this comment

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

unused directive

@@ -40,10 +45,31 @@ public void ConfigureServices(IServiceCollection services)
services.AddAppDependencies();
services.AddMvc();
services.AddAutoMapper();
services.AddHangfire((serviceProvider, config) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess so

@@ -60,6 +86,22 @@ public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerF
}

app.UseMvc();

recurringJobManager.AddOrUpdate<JiraJob>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, are you sure about further code placement in ISA.Jira, ISA.Confluence, etc? I supposed that we will have a core job that will ask IKnowledgeProvider for data (is it possible to create a job per each registered provider?). Is it necessary to force providers to depend on Hangfire? Correct me, if I'm wrong.

"ConnectionStrings": {
"MongoDb": ""
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

new line :)

@mmikirtumov
Copy link
Contributor

Related to Issue #14

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants