WIP: Add Tenanted Device Task Configurations #370

Draft
pramilaniroshan wants to merge 7 commits from pramilaniroshan/device-mgt-core:device-tenant-task-configuration into master
Collaborator

This allowing users to configure task frequencies independently for each tenant.

https://roadmap.entgra.net/issues/9181

This allowing users to configure task frequencies independently for each tenant. https://roadmap.entgra.net/issues/9181
pramilaniroshan added 1 commit 8 months ago
03f95e8cdf
Add Tenanted Device Task Configurations
pramilaniroshan added 1 commit 7 months ago
rajitha reviewed 5 months ago
String deviceType = entry.getKey();
int frequency = entry.getValue();
// Create a JsonObject for the current device type
JsonObject deviceObject = new JsonObject();
Collaborator

Move declaration out side the loop.

Move declaration out side the loop.
Poster
Collaborator

this deviceObject should be created for each entry in the deviceFrequencies map; otherwise,it'll adding the same deviceObject/frequencies for all device types

this deviceObject should be created for each entry in the deviceFrequencies map; otherwise,it'll adding the same deviceObject/frequencies for all device types
pramilaniroshan marked this conversation as resolved
rajitha reviewed 5 months ago
private static final Log log = LogFactory.getLog(DeviceTaskConfigurationServiceImpl.class);
private final MetadataDAO metadataDAO;
Collaborator

Use MetadataManagementService instead of MetadataDAO.

Use MetadataManagementService instead of MetadataDAO.
pramilaniroshan marked this conversation as resolved
rajitha reviewed 5 months ago
DeviceManagementProviderService dms = DeviceManagementDataHolder.getInstance().getDeviceManagementProvider();
List<DeviceType> deviceTypes = dms.getDeviceTypes();
for (DeviceType deviceType : deviceTypes) {
deviceFrequencies.put(deviceType.getName(), getDefaultTaskFrequency(dms, deviceType.getName()));
Collaborator

Remove the logic and just call the addTaskFrequency method.

public void addDefaultTaskFrequency(int tenantId) {

int defaultFrequency = getDefaultTaskFrequency(dms, deviceType.getName());
addTaskFrequency(tenantId, deaultFrequency);
	
}
Remove the logic and just call the addTaskFrequency method. ``` public void addDefaultTaskFrequency(int tenantId) { int defaultFrequency = getDefaultTaskFrequency(dms, deviceType.getName()); addTaskFrequency(tenantId, deaultFrequency); } ```
Poster
Collaborator

what's the reason for this? with this it's bit clean method when calling

what's the reason for this? with this it's bit clean method when calling
Collaborator

Logic is repeating in both methods

Logic is repeating in both methods
pramilaniroshan marked this conversation as resolved
rajitha reviewed 5 months ago
}
@Override
public void completedServerStartup() {
Collaborator

In here, frequencies are setting back to the default frequencies on every server startup. Isn't this an issue?

In here, frequencies are setting back to the default frequencies on every server startup. Isn't this an issue?
Poster
Collaborator

No. we checking if task frequencies already added or not, this run when migration happen

if (!metadataDAO.isExist(tenantId, MetadataConstants.DEVICE_TASK_FREQUENCY))

No. we checking if task frequencies already added or not, this run when migration happen if (!metadataDAO.isExist(tenantId, MetadataConstants.DEVICE_TASK_FREQUENCY))
Collaborator

Got it :)

Got it :)
rajitha marked this conversation as resolved
tcdlpds requested changes 5 months ago
this.metadataDAO = MetadataManagementDAOFactory.getMetadataDAO();
}
private void addMetadataEntry(int tenantId, Metadata metadata, String key) throws MetadataManagementDAOException {
Owner

Add Java Doc comment

Add Java Doc comment
pramilaniroshan marked this conversation as resolved
metadata.setMetaValue(jsonObject.toString());
return metadata;
}
Owner

Remove unnecessary new line

Remove unnecessary new line
pramilaniroshan marked this conversation as resolved
}
private int getDefaultTaskFrequency(DeviceManagementProviderService dms, String deviceType) {
Owner

Add Java Doc comment

Add Java Doc comment
pramilaniroshan marked this conversation as resolved
private int getDefaultTaskFrequency(DeviceManagementProviderService dms, String deviceType) {
OperationMonitoringTaskConfig operationMonitoringTaskConfig = dms.getDeviceMonitoringConfig(deviceType);
Owner

Can 'operationMonitoringTaskConfig' become null, if so handle it here.

If it is not, do we need to have a separate method here? Can't we get the value from following way?

dms.getDeviceMonitoringConfig(deviceType).getFrequency()

Can 'operationMonitoringTaskConfig' become null, if so handle it here. If it is not, do we need to have a separate method here? Can't we get the value from following way? dms.getDeviceMonitoringConfig(deviceType).getFrequency()
Poster
Collaborator

I added separate method because it's improved readability and maintainability

I added separate method because it's improved readability and maintainability
pramilaniroshan marked this conversation as resolved
for (DeviceType deviceType : deviceTypes) {
deviceFrequencies.put(deviceType.getName(), frequency);
}
MetadataManagementDAOFactory.beginTransaction();
Owner

This line should be the first line of a try-catch block. If there is an error because of issue in upper lines then finally block will execute without an open connection.

Further, when you use meta service, you will not need to have this logic.

This line should be the first line of a try-catch block. If there is an error because of issue in upper lines then finally block will execute without an open connection. Further, when you use meta service, you will not need to have this logic.
pramilaniroshan marked this conversation as resolved
// Retrieve the frequency for the given device type
return deviceFrequencyMap.get(deviceType).getFrequency();
} else {
throw new MetadataManagementException("Device type not found: " + deviceType);
Owner

Log the error and throw

Log the error and throw
pramilaniroshan marked this conversation as resolved
}
@Override
public void completedServerStartup() {
Owner

Can't we use already existing 'completedServerStartup' listener?

Can't we use already existing 'completedServerStartup' listener?
Poster
Collaborator

yes we can, but i decided to create a new one with new thread

yes we can, but i decided to create a new one with new thread
pramilaniroshan marked this conversation as resolved
}
}
} catch (MetadataManagementException e) {
String msg = "Error occurred while adding default task frequency metadata entry.";
Owner

It is not required to assign the log message to variable since there is only one usage.

It is not required to assign the log message to variable since there is only one usage.
pramilaniroshan marked this conversation as resolved
log.error("Error occurred while trying to get the available tenants " +
"from device manager provider service.", e);
} catch (MetadataManagementException e) {
String msg = "Error occurred while getting task frequency metadata entry.";
Owner

It is not required to assign the log message to variable since there is only one usage.

It is not required to assign the log message to variable since there is only one usage.
pramilaniroshan marked this conversation as resolved
log.error("Error occurred while trying to add the operations to " +
"device to retrieve device details.", e);
} catch (MetadataManagementException e) {
String msg = "Error occurred while getting task frequency metadata entry.";
Owner

It is not required to assign the log message to variable since there is only one usage.

It is not required to assign the log message to variable since there is only one usage.
pramilaniroshan marked this conversation as resolved
tcdlpds requested changes 5 months ago
}
try {
deviceTaskConfigurationService.addDefaultTaskFrequency(MultitenantConstants.SUPER_TENANT_ID);
Tenant[] tenantArray = realmService.getTenantManager().getAllTenants();
Owner

From this call, doesn't it return super tenant details?

From this call, doesn't it return super tenant details?
Poster
Collaborator

No. It's doesn't return super tenant details

No. It's doesn't return super tenant details
pramilaniroshan marked this conversation as resolved
pramilaniroshan added 1 commit 5 months ago
pramilaniroshan added 1 commit 5 months ago
pramilaniroshan changed title from Add Tenanted Device Task Configurations to WIP: Add Tenanted Device Task Configurations 5 months ago
pramilaniroshan force-pushed device-tenant-task-configuration from e1066fff23 to 0fdeec4c5f 5 months ago
pramilaniroshan changed title from WIP: Add Tenanted Device Task Configurations to Add Tenanted Device Task Configurations 5 months ago
pramilaniroshan changed title from Add Tenanted Device Task Configurations to WIP: Add Tenanted Device Task Configurations 5 months ago
pahansith requested changes 5 months ago
getDeviceTaskManagerService();
OperationMonitoringTaskConfig operationMonitoringTaskConfig = deviceManagementService.
getOperationMonitoringConfig();
TaskManagementService taskManagementService= DeviceManagementDataHolder.getInstance().getTaskManagementService();
Owner

Format code

Format code
}
}
private void startTasksForTenant(Map<String, OperationMonitoringTaskConfig> deviceConfigMap,DeviceTaskManagerService deviceTaskManagerService) throws DeviceMgtTaskException {
Owner

Format code

Format code
private final MetadataManagementService metadataManagementService;
private static DeviceTaskConfigurationServiceImpl instance;
Owner

Make volatile

Make volatile
*/
void updateTask(String deviceType, OperationMonitoringTaskConfig operationMonitoringTaskConfig)
throws DeviceMgtTaskException;
void updateTask(int taskId,DeviceTaskManagerWrapper deviceTaskManagerWrapper)
Owner

Format code

Format code
try {
PrivilegedCarbonContext.startTenantFlow();
PrivilegedCarbonContext.getThreadLocalCarbonContext().setTenantId(tenantId, true);
this.executeTask(operationMonitoringTaskConfig, startupOperationConfig, Utils.getTenantedTaskFrequency(tenantId, deviceType));
Owner

Format code

Format code
public boolean isTaskExist(String taskName) throws TaskManagementException {
int tenantId = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantId();
try {
TaskManagementDAOFactory.beginTransaction();
Owner

What is the requirement for the transaction scope to read database?

What is the requirement for the transaction scope to read database?
log.error(msg, e);
throw new TenantMgtException(msg, e);
} catch (DeviceManagementException e) {
throw new TenantMgtException("Error occurred while getting DeviceManagementService", e);
Owner

Log the error

Log the error
pramilaniroshan force-pushed device-tenant-task-configuration from 0fdeec4c5f to dd71027f4b 5 months ago
pramilaniroshan added 2 commits 1 week ago

Reviewers

tcdlpds requested changes 5 months ago
pahansith requested changes 5 months ago
This pull request is marked as a work in progress.
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: community/device-mgt-core#370
Loading…
There is no content yet.