Skip to content

Issue 2493 #2522

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

Merged
merged 43 commits into from
Sep 28, 2017
Merged

Issue 2493 #2522

merged 43 commits into from
Sep 28, 2017

Conversation

maria-farooq
Copy link

@maria-farooq maria-farooq commented Sep 26, 2017

Maria Farooq added 30 commits September 11, 2017 15:49
* master:
  Patch for Downloader support for TLSv1.1 and TLSv1.2 This close #2501 This close #RESTCOMM-1095 This close #RESTCOMM-1093
  changed OS name
  Added fix for OSX mount issue
  removed unnesesary error log
  Update Restcomm - Docker Quick Start Guide.adoc
  Update Restcomm - Docker Quick Start Guide.adoc
  Update Restcomm - Docker Quick Start Guide.adoc
  change port 80 to 8080
  manage the version of license-maven-plugin
* master:
  Docker RAM requirements and address matching
  'SECURESSL' if condition rollback
  'SECURESSL' if condition fixed
  :stable removed from supported tags
  Port mappings correction
  'Docker Image in OSX host' removed
* master:
  update sip servlet build no
* master: (34 commits)
  IVR test fix
  fixed class casting errors in different classes
  moved Collected result to mscontrol.api and did proper translation in mediagroup, made IvrEndpointResponse to generic
  changed rvd url
  Recording issue fix
  "Gather" replaced with Verbs.gather
  Hints limits logic change
  Hints limits
  Rename local variable
  Added pr parameter to ASR Signal
  Fix for DTMF from MediaServer
  Fix for DTMF using SIP INFO
  added Timeouts description
  added all languages supported by google
  Added configuration for timeouts, fixed issue with languages in ASR
  Added unit tests
  Fix: Go to the next Verb after silent
  Minor changes to restcomm configuration script
  Ignore RCML from partialCallback
  Remove PartialCallback check. Add FinalResult tests
  ...
allowOnlySuperAdmin();

//Character verification
final Pattern pattern = Pattern.compile(SUB_DOMAIN_NAME_VALIDATION_PATTERN);
Copy link
Contributor

Choose a reason for hiding this comment

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

ca nwe compile the patterns in intialization and reuse them?

Copy link
Author

Choose a reason for hiding this comment

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

done

//Check if domain_name does not already taken inside restcomm by an organization.
Organization organization = organizationsDao.getOrganizationByDomainName(domainName);
if(organization != null){
return status(CONFLICT)
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 this checking its just a preValidation, anc concurrent access is later checked in backend,right? otherwise i dont see how this will enforce a unique cosntraint if concurrent accessed...

Copy link
Author

Choose a reason for hiding this comment

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

this line is not here to prevent concurrency but instead its here to intend to see if domain name is not already taken by another organization in restcomm DB.
p.s. domain name column is unique in organization table so DB concurrency is handled at DB level

if(dnsProvisioningManager == null) {
logger.warn("No DNS provisioning Manager is configured, restcomm will not make any queries to DNS server.");
organization = new Organization(Sid.generate(Sid.Type.ORGANIZATION), domainName, DateTime.now(), DateTime.now(), Organization.Status.ACTIVE);
organizationsDao.addOrganization(organization);
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to do this? what will be the impl of this DAO? shouldnt we send an error response.

Copy link
Author

Choose a reason for hiding this comment

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

yes it does, this is to cover that restcomm that is not integrated with a DNS server can also create organization, for example a local deployment etc.

}else {
//for example hosted zone id of domain restcomm.com or others. if not provided then default will be used as per configuration
String hostedZoneId = data.getFirst("HostedZoneId");
if(dnsProvisioningManager.doesResourceRecordAlreadyExists(domainName, hostedZoneId)){
Copy link
Contributor

@jaimecasero jaimecasero Sep 26, 2017

Choose a reason for hiding this comment

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

again, is there any kind of backend transaction happening here? i didnt see any transaction at dao impl level, is this at amazon backend level? shouldnt we synchronize the conurrent acces to ensure this kind of unique constraints are safe?

Even if we use any local sync means,what will happen if the concurrent requests are processed in different cluster nodes?Is there any LB affinity per accoutn in place to ensure they will reach same cluster node?

Copy link
Author

Choose a reason for hiding this comment

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

This particular line is making sure if new domain name is available on dns/amazon-roue53 side

}

@Path("/{domainName}")
@PUT
Copy link
Contributor

Choose a reason for hiding this comment

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

why post and put have same implementation?

Copy link
Author

Choose a reason for hiding this comment

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

on this particular mentioned path both PUT and POST have same implementation i.e. used to create new organization. Later in next sprint we will add another POST method for updating organization

@maria-farooq maria-farooq merged commit 3290bc6 into master Sep 28, 2017
@jaimecasero jaimecasero deleted the issue-2493 branch April 11, 2018 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants