-
Notifications
You must be signed in to change notification settings - Fork 81
Use generic common method to generate message IDs #9005
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
Conversation
@@ -36,12 +35,11 @@ public PipeLineResult processPipeLine(PipeLine pipeLine, String messageId, Messa | |||
// Reset the PipeLineSession and store the message and its id in the session | |||
if (messageId == null) { | |||
// This should not be touched anymore | |||
messageId = MessageUtils.generateMessageId(); | |||
log.error("messageId not set, creating synthetic id [{}]", messageId); | |||
throw new PipeRunException(null, "Pipeline of adapter ["+ pipeLine.getAdapter().getName()+"] received null message id"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misschien dat 'unable to process message without messageID' handiger is?
core/src/main/java/org/frankframework/scheduler/job/SendMessageJob.java
Outdated
Show resolved
Hide resolved
@@ -68,7 +68,7 @@ public class MessageUtils { | |||
|
|||
public static final String JSON_TEMPLATE_VALUE_QUOTED = "{\"%s\": \"%s\"}"; | |||
public static final String JSON_TEMPLATE_VALUE_UNQUOTED = "{\"%s\": %s}"; | |||
public static final String GENERATED_MESSAGE_ID_PREFIX = "synthetic-message-id-"; | |||
public static final String FALLBACK_MESSAGE_ID_PREFIX = "fallback-message-id-"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ik weet niet hoe handig het is deze aan te passen, ik vrees dat dit hard in de larva scenarios staat en niet makkelijk geignored kan worden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ik dacht dat die message IDs sowieso al genegeerd zouden worden omdat ze altijd anders zijn. We zullen zien. Als het breekt, draait ik het terug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Het lijkt geen probleem te zijn in de Larva scenarios maar ik ga hem toch anders noemen. "Fallback" klinkt me toch te negatief, alsof iemand vergeten was een msg-id mee te sturen, terwijl het eigenlijk een normale situatie is in geval van het aanroepen van een sub-adapter.
…eJob.java Co-authored-by: Niels Meijer <nielsmeijer@hotmail.com>
@@ -425,6 +426,7 @@ private static void setupChildSession(PipeLineSession session, ParameterValueLis | |||
if (correlationId != null) { | |||
childSession.put(PipeLineSession.CORRELATION_ID_KEY, correlationId); | |||
} | |||
childSession.put(PipeLineSession.MESSAGE_ID_KEY, MessageUtils.generateMessageId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Men zou deze dus kunnen overschrijven net als het correlatie id, al staat dat expliciet er in met een != check en deze gaat via de putAll hier onder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ik vind deze en die van de ibislocalsender nog een beetje raar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
De putAll is alleen voor de parameters.
Je zou inderdaad met een parameter het messageId en correlationId kunnen overschrijven -- of dat goed is of niet weet ik niet, maar dat was wel al mogelijk.
De reden dat ik hier nu zelf een message-id zet is ergens onderscheid te kunnen blijven maken tussen de gevallen waar we zelf een nieuwe aanroep doen en een daarom een message-id genereren, en de gevallen waar we zonder message-id worden aangeroepen.
(Zoals een ApiListener aanroep die binnenkomt zonder message-id. Dan wordt in de Receiver uiteindelijk een message-id gegenereerd met prefix fallback-message-id
zodat zichtbaar is dat het extern geinitieerd is, maar er geen extern message-id is aangeleverd. Is het prefix FF-MSG
dan is zichtbaar dat het bericht binnen het FF geinitieerd is).
Misschien dat dat onderscheid nergens nodig is, maar het leek mij dat het zinvol kon zijn bij het debuggen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Het is nu een pijnpunt dat wanneer een subadapter aangereopen wordt, in combinatie met een iterating pipe, dat het 2e bericht geflagged wordt als duplicaat omdat ze het zelfde id krijgen / overnemen uit de parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nu met dit PR, of nu in master / latest stable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ik snap het pijnpunt niet helemaal omdat de senders die een subadapter aanroepen, volgens mij nu helemaal geen message-id meegeven in de session? Dus ik weet niet zeker waar het duplicate message id in zit.
Tenzij de IteratorPipe het message-id zelf als parameter zet.
Maar dan is dat een probleem in de pipeline-configuration.
Toch?
Wat mis ik, wat zie ik over het hoofd?
|
No description provided.